[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