Bug 182400 – [CMake] Add ENABLE_ADDRESS_SANITIZER to make it easier to build with asan support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 332896 [details]
Patch

[Konstantin Tokarev]

FWIW, I use ECMEnableSanitizers.cmake

https://api.kde.org/ecm/module/ECMEnableSanitizers.html

[Michael Catanzaro]

(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.

[Konstantin Tokarev]

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

[Konstantin Tokarev]

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

[Michael Catanzaro]

(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.

[Konstantin Tokarev]

(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.

[Michael Catanzaro]

Created attachment 333118 [details]
Patch

[Michael Catanzaro]

(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).

[Konstantin Tokarev]

(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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[WebKit Commit Bot]

Comment on attachment 333118 [details]
Patch

Clearing flags on attachment: 333118

Committed r228134: <https://trac.webkit.org/changeset/228134>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37252242>

[Michael Catanzaro]

I forgot to test if the change to ${CMAKE_THREAD_LIBS_INIT} actually works (it doesn't).

[Michael Catanzaro]

(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.

[Michael Catanzaro]

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. ;)

[Michael Catanzaro]

Committed r228144: <https://trac.webkit.org/changeset/228144>

[Konstantin Tokarev]

(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

[Adrian Perez]

(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

[Adrian Perez]

(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!

[Michael Catanzaro]

(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.

[Michael Catanzaro]

Committed r228293: <https://trac.webkit.org/changeset/228293>

[Loïc Yhuel]

(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.