Add ENABLE_ADDRESS_SANITIZER to make it easier to build with asan support. We should probably investigate adding options for tsan and ubsan as well, if those work well with WebKit. I see clang also has a separate memory sanitizer for detecting uninitialized memory, but looks like that hasn't made it to GCC yet.
Created attachment 332896 [details] Patch
FWIW, I use ECMEnableSanitizers.cmake https://api.kde.org/ecm/module/ECMEnableSanitizers.html
(In reply to Konstantin Tokarev from comment #2) > FWIW, I use ECMEnableSanitizers.cmake > > https://api.kde.org/ecm/module/ECMEnableSanitizers.html Problem with that is it creates options with weird names, and it uses string options instead of simple boolean options.
Comment on attachment 332896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332896&action=review > Source/cmake/WebKitCompilerFlags.cmake:163 > + set(CMAKE_C_FLAGS "-fno-omit-frame-pointer ${CMAKE_C_FLAGS} -fsanitize=address") I think we should use WEBKIT_(PREPEND|APPEND)_GLOBAL_CXX_FLAGS here
Comment on attachment 332896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332896&action=review >> Source/cmake/WebKitCompilerFlags.cmake:163 >> + set(CMAKE_C_FLAGS "-fno-omit-frame-pointer ${CMAKE_C_FLAGS} -fsanitize=address") > > I think we should use WEBKIT_(PREPEND|APPEND)_GLOBAL_CXX_FLAGS here Also, aforementioned module enables -fno-optimize-sibling-calls for ASAN https://github.com/KDE/extra-cmake-modules/blob/master/modules/ECMEnableSanitizers.cmake
(In reply to Konstantin Tokarev from comment #4) > I think we should use WEBKIT_(PREPEND|APPEND)_GLOBAL_CXX_FLAGS here Yes, probably, but it feels weird to use that for compiler flags when we don't have any way to do so for linker flags. CMake provides CheckCCompilerFlag and CheckCXXCompilerFlag modules, but not any to check for supported linker flags (which would be hard to do). And the build will fail for sure if one of the linker flags is unsupported. I guess we can do it for compiler flags but not linker flags, though...? (In reply to Konstantin Tokarev from comment #5) > Also, aforementioned module enables -fno-optimize-sibling-calls for ASAN > > https://github.com/KDE/extra-cmake-modules/blob/master/modules/ > ECMEnableSanitizers.cmake Let's do that then, thanks.
(In reply to Michael Catanzaro from comment #6) > (In reply to Konstantin Tokarev from comment #4) > > I think we should use WEBKIT_(PREPEND|APPEND)_GLOBAL_CXX_FLAGS here > > Yes, probably, but it feels weird to use that for compiler flags when we > don't have any way to do so for linker flags. CMake provides > CheckCCompilerFlag and CheckCXXCompilerFlag modules, but not any to check > for supported linker flags (which would be hard to do). I think it's possible to avoid checking linker flags: * -pthread should be replaced with ${CMAKE_THREAD_LIBS_INIT} as we already do elsewhere * if compiler supports sanitizer flag, it's reasonable to expect that linker flag is supported too. But it means we should check test result variables, which are currently implementation detail. Not pretty, but I think not too ugly at the same time Anyway, checking at least compiler flags is strictly better than no check, especially for other kinds of sanitizers than ASAN > And the build will > fail for sure if one of the linker flags is unsupported. I guess we can do > it for compiler flags but not linker flags, though...? > > (In reply to Konstantin Tokarev from comment #5) > > Also, aforementioned module enables -fno-optimize-sibling-calls for ASAN > > > > https://github.com/KDE/extra-cmake-modules/blob/master/modules/ > > ECMEnableSanitizers.cmake > > Let's do that then, thanks.
Created attachment 333118 [details] Patch
(In reply to Konstantin Tokarev from comment #7) > * if compiler supports sanitizer flag Turns out that's not possible to check; see the note in my updated patch. We can check for -fno-omit-frame-pointer and -fno-optimize-sibling-calls, though. I guess we can rely on developer common sense not to set ENABLE_ADDRESS_SANITIZER if the compiler doesn't support -fsanitize=address. And the option is already guarded by if (COMPILER_IS_GCC_OR_CLANG).
(In reply to Michael Catanzaro from comment #9) > (In reply to Konstantin Tokarev from comment #7) > > * if compiler supports sanitizer flag > > Turns out that's not possible to check; see the note in my updated patch. Oh. Seems like CheckCXXSourceCompiles is required to do that. >We > can check for -fno-omit-frame-pointer and -fno-optimize-sibling-calls, > though. I guess we can rely on developer common sense not to set > ENABLE_ADDRESS_SANITIZER if the compiler doesn't support -fsanitize=address. Indeed, it probably isn't worth extra complexity > And the option is already guarded by if (COMPILER_IS_GCC_OR_CLANG). FWIW, there are a few compilers besides clang which support gcc-compatible options and disguise themselves as gcc. But this isn't really a problem.
Comment on attachment 333118 [details] Patch Attachment 333118 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6370585 New failing tests: media/modern-media-controls/tracks-support/tracks-support-show-panel-after-dragging-controls.html
Created attachment 333122 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 333118 [details] Patch Clearing flags on attachment: 333118 Committed r228134: <https://trac.webkit.org/changeset/228134>
All reviewed patches have been landed. Closing bug.
<rdar://problem/37252242>
I forgot to test if the change to ${CMAKE_THREAD_LIBS_INIT} actually works (it doesn't).
(In reply to Michael Catanzaro from comment #16) > I forgot to test if the change to ${CMAKE_THREAD_LIBS_INIT} actually works > (it doesn't). It's an empty string, at least when used in WebKitCompilerFlags.cmake.
It's an empty string even when I add a call to find_package(Threads) directly above. I'm going to change it back to hardcoded "-pthread" for now. Suggestions welcome. ;)
Committed r228144: <https://trac.webkit.org/changeset/228144>
(In reply to Michael Catanzaro from comment #18) > It's an empty string even when I add a call to find_package(Threads) > directly above. I'm going to change it back to hardcoded "-pthread" for now. > Suggestions welcome. ;) Weird, it's -lpthread here
(In reply to Konstantin Tokarev from comment #20) > (In reply to Michael Catanzaro from comment #18) > > It's an empty string even when I add a call to find_package(Threads) > > directly above. I'm going to change it back to hardcoded "-pthread" for now. > > Suggestions welcome. ;) > > Weird, it's -lpthread here Both “-lpthread” are “-pthread” are valid options: the first will *only* pull in “libpthread.{so,a}” at link time, while “-pthread” will do that *and* also ensure that other compiler and linker flags needed for threads to work properly. Using only “-lpthread” may not work in some systems. TL;DR: Use “-pthread” whenever available, fall-back to “-lpthread” when not. More info: https://stackoverflow.com/questions/23250863/difference-between-pthread-and-lpthread-while-compiling#23251828
(In reply to Adrian Perez from comment #21) > (In reply to Konstantin Tokarev from comment #20) > > (In reply to Michael Catanzaro from comment #18) > > > It's an empty string even when I add a call to find_package(Threads) > > > directly above. I'm going to change it back to hardcoded "-pthread" for now. > > > Suggestions welcome. ;) > > > > Weird, it's -lpthread here > > Both “-lpthread” are “-pthread” are valid options: the first will *only* > pull in “libpthread.{so,a}” at link time, while “-pthread” will do that > *and* also ensure that other compiler and linker flags needed for threads > to work properly. Using only “-lpthread” may not work in some systems. > > TL;DR: Use “-pthread” whenever available, fall-back to “-lpthread” when not. > > More info: > > > https://stackoverflow.com/questions/23250863/difference-between-pthread-and- > lpthread-while-compiling#23251828 Also, in case it wasn't completely clear: * If the compiler supports “-pthread”, it MUST be passed also to the compiler (not only to the linker). * When using “-pthread”, it is not needed to pass “-lpthread” to the linker. * Some compilers/platforms might not support “-pthread”, so we should check whether it's supported and not just blindly pass the flag. I hope this helps!
(In reply to Adrian Perez from comment #22) > Also, in case it wasn't completely clear: > > * If the compiler supports “-pthread”, it MUST be passed also to the > compiler (not only to the linker). Here it's really needed in the linker flags. I guess I should change it to -lpthread? > * Some compilers/platforms might not support “-pthread”, so we should > check whether it's supported and not just blindly pass the flag. That's the problem, ${CMAKE_THREAD_LIBS_INIT} doesn't work for me, even after calling find_package(Threads), so there's not any good way to test it.
Committed r228293: <https://trac.webkit.org/changeset/228293>
(In reply to Michael Catanzaro from comment #23) > That's the problem, ${CMAKE_THREAD_LIBS_INIT} doesn't work for me, even > after calling find_package(Threads), so there's not any good way to test it. This is a common problem for libpthread/libdl checks, they often check symbols overriden by libasan. With -fsanitize=address, "CHECK_SYMBOL_EXISTS(pthread_create pthread.h CMAKE_HAVE_LIBC_CREATE)" succeeds because "pthread_create" is in libasan.so. But I'm not sure how it works here, since CMAKE_EXE_LINKER_FLAGS should be set after the test. Perhaps it depends on whether the -fsanitize=address is already in the cache or not.