Bug 197920

Created attachment 369974 [details] patch

Attachment 369974 [details] did not pass style-queue: ERROR: Source/cmake/WebKitCompilerFlags.cmake:231: One space between command "endif" and its parentheses, should be "endif (" [whitespace/parentheses] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 369975 [details] style fix

Comment on attachment 369975 [details] style fix View in context: https://bugs.webkit.org/attachment.cgi?id=369975&action=review Cool, but needs some further discussion, particularly relating to -fvisibility=hidden. > ChangeLog:9 > + cfi-debug is using cfi recovery flags to not trap on errors and continue exectution. execution > Source/cmake/OptionsGTK.cmake:379 > +if (ENABLED_HIDDEN_VISIBILITY) > + SET_AND_EXPOSE_TO_BUILD(USE_EXPORT_MACROS ON) > +endif () If you got a build to work with -fvisibility=hidden, then we should switch to using -fvisibility=hidden by default, because that would be wonderful. I doubt you actually got this working properly, though. There are going to be problems. E.g. global/singleton templates declared in JSC or lower layers need to be exported weak symbols to ensure the instantiations are actually shared between libjavascriptcoregtk and libwebkit2gtk. Specifically, I worry that PerProcess<> objects declared in JSC, WTF, or bmalloc are now going to actually exist twice, once for each library. > Source/cmake/WebKitCompilerFlags.cmake:221 > + set(CFI_FLAGS "-fno-sanitize-trap=cfi -fsanitize-recover=cfi") cfi-debug is useful why? You really want to continue execution past a CFI issue? > Source/cmake/WebKitCompilerFlags.cmake:225 > + set(CFI_FLAGS "${CFI_FLAGS} -fno-sanitize=cfi-derived-cast,cfi-unrelated-cast -fsanitize-blacklist=${TOOLS_DIR}/cfi/blacklist.txt") So this blacklist won't work at all in tarball builds. I suppose that's not the end of the world, but it'd be nice to have a failure case nicer than "whoops the file doesn't exist!" It would be highly-desirable to make the code work without the blacklist, by changing the code to avoid constructs that trigger CFI. Especially since it looks like there's only a couple places that need to be looked at. I'm very skeptical of the need to disable cfi-derived-cast and cfi-unrelated-cast checks. This suggests we have problematic code that should be fixed. Of course, it makes sense to allow running WebKit with CFI now, with a mind towards improving this situation in the future, so that shouldn't be a blocker. > Source/cmake/WebKitCompilerFlags.cmake:247 > + string(REGEX MATCHALL "-fvisibility=hidden" ENABLED_HIDDEN_VISIBILITY ${CMAKE_CXX_FLAGS}) You are manually passing -fvisibility=hidden to test this with GTK? Surely that shouldn't be needed.

BTW: we could try turning on -fvisibility=hidden for WPE (it actually used to use it!), but I don't think it's possible for GTK, at least not without a lot of thinking.

Comment on attachment 369975 [details] style fix View in context: https://bugs.webkit.org/attachment.cgi?id=369975&action=review >> ChangeLog:9 >> + cfi-debug is using cfi recovery flags to not trap on errors and continue exectution. > > execution Oops, will fix >> Source/cmake/OptionsGTK.cmake:379 >> if (ENABLED_COMPILER_SANITIZERS) >> set(ENABLE_INTROSPECTION OFF) >> endif () >> >> +if (ENABLED_HIDDEN_VISIBILITY) >> + SET_AND_EXPOSE_TO_BUILD(USE_EXPORT_MACROS ON) >> +endif () > > If you got a build to work with -fvisibility=hidden, then we should switch to using -fvisibility=hidden by default, because that would be wonderful. > > I doubt you actually got this working properly, though. There are going to be problems. E.g. global/singleton templates declared in JSC or lower layers need to be exported weak symbols to ensure the instantiations are actually shared between libjavascriptcoregtk and libwebkit2gtk. Specifically, I worry that PerProcess<> objects declared in JSC, WTF, or bmalloc are now going to actually exist twice, once for each library. We were initially testing out hidden visibility and CFI in just jsc, I haven't tested cfi with all of webkit much yet. I tried out using hidden visibility with all of GTK. It currently compiles and runs layout tests, there does seem to be a bunch issues that need sorting out. The issues you mentioned sound like they would also impact playstation so it would be good for us to help sort them out. I created a new bug for that https://bugs.webkit.org/show_bug.cgi?id=198093. >> Source/cmake/WebKitCompilerFlags.cmake:221 >> + set(CFI_FLAGS "-fno-sanitize-trap=cfi -fsanitize-recover=cfi") > > cfi-debug is useful why? You really want to continue execution past a CFI issue? We've hit some cases where I believe what's happening is the compiler sees code that will allways trigger CFI and just emits an Illegal instruction instead of CFI handlers. Using recovery flags makes it much easier to figure out what error was hit instead of having to figure out why the compiler suddenly stopped compiling out a function. This isn't meant to be used for production builds, just for discovering CFI failure causes. >> Source/cmake/WebKitCompilerFlags.cmake:225 >> + set(CFI_FLAGS "${CFI_FLAGS} -fno-sanitize=cfi-derived-cast,cfi-unrelated-cast -fsanitize-blacklist=${TOOLS_DIR}/cfi/blacklist.txt") > > So this blacklist won't work at all in tarball builds. I suppose that's not the end of the world, but it'd be nice to have a failure case nicer than "whoops the file doesn't exist!" It would be highly-desirable to make the code work without the blacklist, by changing the code to avoid constructs that trigger CFI. Especially since it looks like there's only a couple places that need to be looked at. > > I'm very skeptical of the need to disable cfi-derived-cast and cfi-unrelated-cast checks. This suggests we have problematic code that should be fixed. Of course, it makes sense to allow running WebKit with CFI now, with a mind towards improving this situation in the future, so that shouldn't be a blocker. I'll look at getting it working for tarball builds. It would be nice to get the cast checks enabled in the future. It seems like historically there's been issues cfi-derived-cast or cfi-unrelated-cast would catch https://trac.webkit.org/changeset/145013/webkit. >> Source/cmake/WebKitCompilerFlags.cmake:247 >> + string(REGEX MATCHALL "-fvisibility=hidden" ENABLED_HIDDEN_VISIBILITY ${CMAKE_CXX_FLAGS}) > > You are manually passing -fvisibility=hidden to test this with GTK? Surely that shouldn't be needed. Yeah this isn't needed if we get it enabled by default for GTK.