[CMake] Add support for building with Clang's AddressSanitizer
Created attachment 229818 [details] Patch
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.
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?
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.
(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.
Created attachment 229986 [details] Patch Made ENABLE_CLANG_ASAN non-advanced configuration option.
Created attachment 230260 [details] Patch Fixes introspection linker flags.
Created attachment 230291 [details] Patch Rebased patch.
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.
Created attachment 230469 [details] Patch
Created attachment 230506 [details] Patch Fixed enabling sanitizers when using GCC.
This would be nice to have....
Created attachment 251831 [details] Patch
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
Created attachment 254486 [details] Patch Avoids adding a new CMake option by deducing any enabled sanitizer from CMAKE_CXX_FLAGS.
This looks like an easy review, could anyone take a look?
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?
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.
Created attachment 255176 [details] Patch Includes previous LDFLAGS when invoking generate-gtkdoc.
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?
Comment on attachment 255176 [details] Patch The GTK EWS seems that failed to build with this patch: https://webkit-queues.appspot.com/results/4640530692571136
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.
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.
(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. :)
(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.
(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.
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).
Comment on attachment 255316 [details] Patch Clearing flags on attachment: 255316 Committed r185823: <http://trac.webkit.org/changeset/185823>
All reviewed patches have been landed. Closing bug.