Bug 131941 – [CMake] Add support for building with various sanitizer tools

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[CMake] Add support for building with Clang's AddressSanitizer

[Zan Dobersek]

Created attachment 229818 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 229818 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229818&action=review

> Source/cmake/OptionsCommon.cmake:39
> +mark_as_advanced(ENABLE_CLANG_ASAN)

Are you sure you need this? The idea of having an OPTION() is to let it be easily configurable via ccmake or cmake-gui.

[Martin Robinson]

Comment on attachment 229818 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229818&action=review

> Source/cmake/OptionsCommon.cmake:37
> +set(ENABLED_CLANG_SANITIZERS "")

What's the idea with having ENABLED_CLANG_SANITIZERS as a variable. Do you want the user to be able to override it?

[Zan Dobersek]

Comment on attachment 229818 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229818&action=review

>> Source/cmake/OptionsCommon.cmake:37
>> +set(ENABLED_CLANG_SANITIZERS "")
> 
> What's the idea with having ENABLED_CLANG_SANITIZERS as a variable. Do you want the user to be able to override it?

No, it's meant to be filled with space-separated names of the enabled sanitizers. When additional sanitizers are supported the variable would contain something like ' address thread' when AddressSanitizer and ThreadSanitizer are enabled in parallel, achievable by passing -DENABLE_CLANG_ASAN=ON and -DENABLE_CLANG_TSAN=ON to CMake.

>> Source/cmake/OptionsCommon.cmake:39
>> +mark_as_advanced(ENABLE_CLANG_ASAN)
> 
> Are you sure you need this? The idea of having an OPTION() is to let it be easily configurable via ccmake or cmake-gui.

It depends on how easily configurable we want this option to be. I think the option has limited purpose and wouldn't be widely used, hence hiding it. But I don't have any strong opinion on this.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #4)
> (From update of attachment 229818 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229818&action=review
> >> Source/cmake/OptionsCommon.cmake:39
> >> +mark_as_advanced(ENABLE_CLANG_ASAN)
> > 
> > Are you sure you need this? The idea of having an OPTION() is to let it be easily configurable via ccmake or cmake-gui.
> 
> It depends on how easily configurable we want this option to be. I think the option has limited purpose and wouldn't be widely used, hence hiding it. But I don't have any strong opinion on this.

I'd say you should remove this line for consistency: the usual idiom is to have all OPTION()s available in "normal" mode for users to set.

[Zan Dobersek]

Created attachment 229986 [details]
Patch

Made ENABLE_CLANG_ASAN non-advanced configuration option.

[Zan Dobersek]

Created attachment 230260 [details]
Patch

Fixes introspection linker flags.

[Zan Dobersek]

Created attachment 230291 [details]
Patch

Rebased patch.

[Zan Dobersek]

Comment on attachment 230291 [details]
Patch

This is just over-engineered.

I'll re-check if it's possible to build simply by listing the -fsanitize option in CFLAGS, CXXFLAGS and LDFLAGS. If not, a much simpler approach would be to have a single option, named ENABLE_COMPILER_SANITIZERS or similar. It would be set with a comma-separated list of values and would work with both GCC and Clang.

[Zan Dobersek]

Created attachment 230469 [details]
Patch

[Zan Dobersek]

Created attachment 230506 [details]
Patch

Fixed enabling sanitizers when using GCC.

[Michael Catanzaro]

This would be nice to have....

[Zan Dobersek]

Created attachment 251831 [details]
Patch

[Michael Catanzaro]

So the value of the ENABLE_COMPILER_SANITIZERS option is that if you compile with -fsanitize=something and also -DENABLE_COMPILER_SANITIZERS, then we don't add -Wl,--no-undefined to LDFLAGS, which I guess breaks the sanitizers? Wouldn't it be better to check for -fsanitize in CXXFLAGS and Do The Right Thing?

Why don't we simply pass all CFLAGS to introspection? That's the usual practice, though admittedly it has led to some interesting issues [1].

[1] https://git.gnome.org/browse/gobject-introspection/commit/?id=d2dce55c971fc44cc327818b0341eb89f1243db6

[Zan Dobersek]

Created attachment 254486 [details]
Patch

Avoids adding a new CMake option by deducing any enabled sanitizer from CMAKE_CXX_FLAGS.

[Michael Catanzaro]

This looks like an easy review, could anyone take a look?

[Martin Robinson]

Comment on attachment 254486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254486&action=review

> Source/PlatformGTK.cmake:28
> +        COMMAND CC=${CMAKE_C_COMPILER} CFLAGS=${CMAKE_C_FLAGS} LDFLAGS=${ENABLED_COMPILER_SANITIZERS} ${CMAKE_SOURCE_DIR}/Tools/gtk/generate-gtkdoc ${_extra_args}

Hrm. Doesn't this override any existing LDFLAGS that the user may be passing?

[Zan Dobersek]

Comment on attachment 254486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254486&action=review

>> Source/PlatformGTK.cmake:28
>> +        COMMAND CC=${CMAKE_C_COMPILER} CFLAGS=${CMAKE_C_FLAGS} LDFLAGS=${ENABLED_COMPILER_SANITIZERS} ${CMAKE_SOURCE_DIR}/Tools/gtk/generate-gtkdoc ${_extra_args}
> 
> Hrm. Doesn't this override any existing LDFLAGS that the user may be passing?

Right, this should be fixed.

[Zan Dobersek]

Created attachment 255176 [details]
Patch

Includes previous LDFLAGS when invoking generate-gtkdoc.

[Martin Robinson]

Comment on attachment 255176 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255176&action=review

> Source/WebKit2/PlatformGTK.cmake:841
> +    COMMAND CC=${CMAKE_C_COMPILER} CFLAGS="-Wno-deprecated-declarations ${ENABLED_COMPILER_SANITIZERS}" LDFLAGS=

Okay. Sorry, I missed this earlier, but why are the sanitizer arguments included in CFLAGS here, but LDFLAGS earlier?

[Carlos Alberto Lopez Perez]

Comment on attachment 255176 [details]
Patch

The GTK EWS seems that failed to build with this patch: https://webkit-queues.appspot.com/results/4640530692571136

[Zan Dobersek]

Comment on attachment 255176 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255176&action=review

>> Source/WebKit2/PlatformGTK.cmake:841
>> +    COMMAND CC=${CMAKE_C_COMPILER} CFLAGS="-Wno-deprecated-declarations ${ENABLED_COMPILER_SANITIZERS}" LDFLAGS=
> 
> Okay. Sorry, I missed this earlier, but why are the sanitizer arguments included in CFLAGS here, but LDFLAGS earlier?

Honestly, I can't tell you for sure, but that was the only possible way to get the thing to not fail.

I'll try to disable the gtk-doc and gobject-introspection generation when enabling compiler sanitizers. At least for starters we can do without those two when testing.

[Zan Dobersek]

Created attachment 255316 [details]
Patch

A nicely reduced patch, though maybe it would be better to have a separate option to completely disable document generation during developer builds.

[Martin Robinson]

(In reply to comment #23)
> Created attachment 255316 [details]
> Patch
> 
> A nicely reduced patch, though maybe it would be better to have a separate
> option to completely disable document generation during developer builds.

We probably don't want to make it too easy to disable gtkdoc generation during developer builds. That is one reason why the documentation used to be broken all the time. :)

[Michael Catanzaro]

(In reply to comment #23)
> Created attachment 255316 [details]
> Patch
> 
> A nicely reduced patch, though maybe it would be better to have a separate
> option to completely disable document generation during developer builds.

Maybe I don't understand, but we shouldn't need any new option. If you use -DENABLE_GTKDOC=OFF, you _should_ get no gtkdoc unless you manually run 'make gtkdoc' (or 'ninja gtkdoc' I guess), since ALL is not passed to add_custom_target(). If gtkdoc is somehow generated when ENABLE_GTKDOC=OFF, then that's just a bug.

[Martin Robinson]

(In reply to comment #25)

> Maybe I don't understand, but we shouldn't need any new option. If you use
> -DENABLE_GTKDOC=OFF, you _should_ get no gtkdoc unless you manually run
> 'make gtkdoc' (or 'ninja gtkdoc' I guess), since ALL is not passed to
> add_custom_target(). If gtkdoc is somehow generated when ENABLE_GTKDOC=OFF,
> then that's just a bug.

See the comment on line 42 of Source/PlatformGTK.cmake. When developer mode is active, we will run some steps of gtkdoc generation to keep the documentation building during development.

[Zan Dobersek]

The build command that I last used:
CC=clang CXX=clang++ CFLAGS="-fsanitize=address -fno-omit-frame-pointer" CXXFLAGS="-stdlib=libc++ -fsanitize=address -fno-omit-frame-pointer" Tools/Scripts/build-webkit --gtk --cmakeargs="-DENABLE_INTROSPECTION=OFF"

Note that this still needs to pass the path to Tools/asan/webkit-asan-ignore.txt through the -fsanitize-blacklist compiler flag, and could use a lower optimization level (-O1 is recommended, -O3 used by default for release builds).

[Zan Dobersek]

Comment on attachment 255316 [details]
Patch

Clearing flags on attachment: 255316

Committed r185823: <http://trac.webkit.org/changeset/185823>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.