Bug 185462 – [WPE] Build cleanly with GCC 8 and ICU 60

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Michael Catanzaro]

WPE should build cleanly with GCC 8 and ICU 60

I haven't tested GTK yet.

[Michael Catanzaro]

Created attachment 339927 [details]
Patch

[Michael Catanzaro]

Comment on attachment 339927 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339927&action=review

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:633
> -#define CREATE_PROTOTYPE_FOR_SIMPLE_TYPE(capitalName, lowerName, properName, instanceType, jsName, prototypeBase) \
> -m_ ## lowerName ## Prototype.set(vm, this, capitalName##Prototype::create(vm, this, capitalName##Prototype::createStructure(vm, this, m_ ## prototypeBase ## Prototype.get()))); \
> -m_ ## properName ## Structure.set(vm, this, instanceType::createStructure(vm, this, m_ ## lowerName ## Prototype.get()));
> +#define CREATE_PROTOTYPE_FOR_SIMPLE_TYPE(capitalName, lowerName, properName, instanceType, jsName, prototypeBase) do { \
> +        m_ ## lowerName ## Prototype.set(vm, this, capitalName##Prototype::create(vm, this, capitalName##Prototype::createStructure(vm, this, m_ ## prototypeBase ## Prototype.get()))); \
> +        m_ ## properName ## Structure.set(vm, this, instanceType::createStructure(vm, this, m_ ## lowerName ## Prototype.get())); \
> +    } while (0);

I can split the JSGlobalObject.cpp changes out into a separate patch, if desired, since this fixes a pretty serious bug. (CREATE_PROTOTYPE_FOR_SIMPLE_TYPE() and VISIT_SIMPLE_TYPE() are used in conditionals without braces, but there are multiple statements, so the second one gets executed unconditionally. Score one for GCC.)

[Michael Catanzaro]

Created attachment 339928 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 339928 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339928&action=review

it looks ok, but i think those extra breaks added to make gcc8 happy should be changed by ASSERT_NOT_REACHED() instead.

> Source/JavaScriptCore/b3/air/AirArg.h:892
> +            break;

I think this one should be ASSERT_NOT_REACHED(); instead of break
Also.. why this change is not needed also in castToType() 20 lines below?

> Source/JavaScriptCore/dfg/DFGDoubleFormatState.h:51
> +        break;

ASSERT_NOT_REACHED();

> Source/JavaScriptCore/dfg/DFGDoubleFormatState.h:61
> +        break;

ASSERT_NOT_REACHED();

> Source/JavaScriptCore/heap/MarkedBlockInlines.h:391
> +                break;

ASSERT_NOT_REACHED();

> Source/JavaScriptCore/heap/MarkedBlockInlines.h:402
> +            break;

ASSERT_NOT_REACHED();

> Source/JavaScriptCore/heap/MarkedBlockInlines.h:414
> +                break;

ASSERT_NOT_REACHED();

> Source/WebCore/css/CSSFontFace.cpp:608
> +        break;

ASSERT_NOT_REACHED();

[Michael Catanzaro]

Sure. It actually probably needs to be RELEASE_ASSERT_NOT_REACHED() for release builds. 

(In reply to Carlos Alberto Lopez Perez from comment #4)
> > Source/JavaScriptCore/b3/air/AirArg.h:892
> > +            break;
> 
> I think this one should be ASSERT_NOT_REACHED(); instead of break
> Also.. why this change is not needed also in castToType() 20 lines below?

Weird. GCC works in mysterious ways. I'll add it there too.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #5)
> Sure. It actually probably needs to be RELEASE_ASSERT_NOT_REACHED() for
> release builds. 

Confirmed.

[Michael Catanzaro]

Committed r231565: <https://trac.webkit.org/changeset/231565>

[Don Olmstead]

Comment on attachment 339928 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339928&action=review

Noticed this issue while rebasing. GCallback shouldn't be in freetype files.

> Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:51
> +        reinterpret_cast<cairo_destroy_func_t>(reinterpret_cast<GCallback>(FT_Done_Face)));

This casts assumes USE(GLIB)

[Adrian Perez]

(In reply to Don Olmstead from comment #8)
> Comment on attachment 339928 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339928&action=review
> 
> Noticed this issue while rebasing. GCallback shouldn't be in freetype files.
> 
> > Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:51
> > +        reinterpret_cast<cairo_destroy_func_t>(reinterpret_cast<GCallback>(FT_Done_Face)));
> 
> This casts assumes USE(GLIB)

Ouch. The double cast should not even be needed. Patch posted in bug #188919