[webkit-reviews] review denied: [Bug 197920] [CMake] Add support for building with CFI : [Attachment 369975] style fix
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed May 15 12:06:37 PDT 2019
Michael Catanzaro <mcatanzaro at igalia.com> has denied Christopher Reid
<chris.reid at sony.com>'s request for review:
Bug 197920: [CMake] Add support for building with CFI
https://bugs.webkit.org/show_bug.cgi?id=197920
Attachment 369975: style fix
https://bugs.webkit.org/attachment.cgi?id=369975&action=review
--- 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.
More information about the webkit-reviews
mailing list