Bug 197920 - [CMake] Add support for building with CFI
Summary: [CMake] Add support for building with CFI
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CMake (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-05-15 11:20 PDT by Christopher Reid
Modified: 2019-05-23 22:12 PDT (History)
6 users (show)

See Also:


Attachments
patch (4.74 KB, patch)
2019-05-15 11:36 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
style fix (4.74 KB, patch)
2019-05-15 11:45 PDT, Christopher Reid
mcatanzaro: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Reid 2019-05-15 11:20:23 PDT
Support building with CFI on clang in cmake builds.
Comment 1 Christopher Reid 2019-05-15 11:36:54 PDT
Created attachment 369974 [details]
patch
Comment 2 Build Bot 2019-05-15 11:39:38 PDT
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.
Comment 3 Christopher Reid 2019-05-15 11:45:52 PDT
Created attachment 369975 [details]
style fix
Comment 4 Michael Catanzaro 2019-05-15 12:06:37 PDT
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.
Comment 5 Michael Catanzaro 2019-05-15 12:27:12 PDT
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 6 Christopher Reid 2019-05-21 16:55:32 PDT
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.