[Webkit-unassigned] [Bug 197920] [CMake] Add support for building with CFI

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=197920

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #369975|review?                     |review-
              Flags|                            |

--- Comment #4 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 369975
  --> https://bugs.webkit.org/attachment.cgi?id=369975
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190515/142acc57/attachment.html>