Bug 185462

Summary: [WPE] Build cleanly with GCC 8 and ICU 60
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WPE WebKitAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: aperez, bugs-noreply, clopez, don.olmstead, mcatanzaro
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch clopez: review+, clopez: commit-queue-

Michael Catanzaro
Reported 2018-05-08 20:26:05 PDT
WPE should build cleanly with GCC 8 and ICU 60 I haven't tested GTK yet.
Attachments
Patch (52.11 KB, patch)
2018-05-08 21:28 PDT, Michael Catanzaro
no flags
Patch (58.20 KB, patch)
2018-05-08 21:56 PDT, Michael Catanzaro
clopez: review+
clopez: commit-queue-
Michael Catanzaro
Comment 1 2018-05-08 21:28:57 PDT
Michael Catanzaro
Comment 2 2018-05-08 21:31:08 PDT
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
Comment 3 2018-05-08 21:56:56 PDT
Carlos Alberto Lopez Perez
Comment 4 2018-05-09 09:08:28 PDT
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
Comment 5 2018-05-09 09:12:01 PDT
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
Comment 6 2018-05-09 09:20:11 PDT
(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
Comment 7 2018-05-09 09:21:23 PDT
Don Olmstead
Comment 8 2018-08-23 18:46:29 PDT
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
Comment 9 2018-08-24 06:14:15 PDT
(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
Note You need to log in before you can comment on or make changes to this bug.