Bug 186957 – Fix ASAN_ENABLED in GCC

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Alicia Boya García]

Currently ASAN_ENABLED only returns true in Clang, since it uses Clang-specific detection features.

[Alicia Boya García]

Created attachment 343408 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 343408 [details]
Patch

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

> Source/WTF/wtf/Compiler.h:161
> + * Therefore we need to give up on the enforcement of ALWAYS_INLINE when bulding with ASAN. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 */

Doesn’t this change disable it for clang too?

[Alicia Boya García]

(In reply to Alexey Proskuryakov from comment #2)
> Comment on attachment 343408 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343408&action=review
> 
> > Source/WTF/wtf/Compiler.h:161
> > + * Therefore we need to give up on the enforcement of ALWAYS_INLINE when bulding with ASAN. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 */
> 
> Doesn’t this change disable it for clang too?

Yes, it does. I'll modify it so that is not the case, since Clang supports that.

I also need to change the definition a little since Clang is not happy with using defined() inside a macro expansion.

In file included from /Volumes/Data/EWS/WebKit/Source/WTF/wtf/unicode/CollatorDefault.cpp:29:
In file included from /Volumes/Data/EWS/WebKit/Source/WTF/config.h:26:
In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphoneos/usr/local/include/wtf/ExportMacros.h:32:
In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphoneos/usr/local/include/wtf/Platform.h:32:
/Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphoneos/usr/local/include/wtf/Compiler.h:150:5: error: macro expansion producing 'defined' has undefined behavior [-Werror,-Wexpansion-to-defined]
#if ASAN_ENABLED
    ^
/Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphoneos/usr/local/include/wtf/Compiler.h:148:23: note: expanded from macro 'ASAN_ENABLED'
#define ASAN_ENABLED (defined(__SANITIZE_ADDRESS__) || COMPILER_HAS_CLANG_FEATURE(address_sanitizer))
                      ^
/Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphoneos/usr/local/include/wtf/Compiler.h:162:98: error: macro expansion producing 'defined' has undefined behavior [-Werror,-Wexpansion-to-defined]
#if !defined(ALWAYS_INLINE) && COMPILER(GCC_OR_CLANG) && defined(NDEBUG) && !COMPILER(MINGW) && !ASAN_ENABLED
                                                                                                 ^
/Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphoneos/usr/local/include/wtf/Compiler.h:148:23: note: expanded from macro 'ASAN_ENABLED'
#define ASAN_ENABLED (defined(__SANITIZE_ADDRESS__) || COMPILER_HAS_CLANG_FEATURE(address_sanitizer))

[Alicia Boya García]

Created attachment 343429 [details]
Patch

[Michael Catanzaro]

Comment on attachment 343429 [details]
Patch

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

> Source/WTF/wtf/Compiler.h:166
> +/* In GCC functions marked with no_sanitize_address cannot call functions that are marked with always_inline and not marked with no_sanitize_address.
> + * Therefore we need to give up on the enforcement of ALWAYS_INLINE when bulding with ASAN. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 */
> +#if !defined(ALWAYS_INLINE) && COMPILER(GCC_OR_CLANG) && defined(NDEBUG) && !COMPILER(MINGW) && !(COMPILER(GCC) && ASAN_ENABLED)

I think it's easier to read without the comment... it should be clear from the condition you've added that ALWAYS_INLINE + GCC + ASAN_ENABLED do not mix..

> Source/WTF/wtf/Vector.h:531
>          ASSERT(buffer());
> +#if COMPILER(GCC)

I would leave a blank line between here.

> PerformanceTests/ChangeLog:8
> +        Updated Compiler.h to reflect changes in WTF.

Hm, I don't think we should do this unless there's a compelling reason, and surely performance tests are not meant to be run under asan. We can't be updating StitchMarker every time

[Alicia Boya García]

Comment on attachment 343429 [details]
Patch

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

>> Source/WTF/wtf/Compiler.h:166
>> +#if !defined(ALWAYS_INLINE) && COMPILER(GCC_OR_CLANG) && defined(NDEBUG) && !COMPILER(MINGW) && !(COMPILER(GCC) && ASAN_ENABLED)
> 
> I think it's easier to read without the comment... it should be clear from the condition you've added that ALWAYS_INLINE + GCC + ASAN_ENABLED do not mix..

The point of the comment is to explain why.

>> Source/WTF/wtf/Vector.h:531
>> +#if COMPILER(GCC)
> 
> I would leave a blank line between here.

I can't see why.

>> PerformanceTests/ChangeLog:8
>> +        Updated Compiler.h to reflect changes in WTF.
> 
> Hm, I don't think we should do this unless there's a compelling reason, and surely performance tests are not meant to be run under asan. We can't be updating StitchMarker every time

So I was supposed to let it stay with an older version of the same code? Why?

[EWS Watchlist]

Comment on attachment 343429 [details]
Patch

Attachment 343429 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8305347

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

[EWS Watchlist]

Created attachment 343442 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 343429 [details]
Patch

Attachment 343429 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8306183

New failing tests:
http/tests/security/local-video-source-from-remote.html

[EWS Watchlist]

Created attachment 343445 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Michael Catanzaro]

(In reply to Alicia Boya García from comment #6)
> >> Source/WTF/wtf/Vector.h:531
> >> +#if COMPILER(GCC)
> > 
> > I would leave a blank line between here.
> 
> I can't see why.

Since it's a huge block of related code, and just one unrelated line, that's where I would personally put the blank line. Anyway, up to you, this is just a style nit.

> >> PerformanceTests/ChangeLog:8
> >> +        Updated Compiler.h to reflect changes in WTF.
> > 
> > Hm, I don't think we should do this unless there's a compelling reason, and surely performance tests are not meant to be run under asan. We can't be updating StitchMarker every time
> 
> So I was supposed to let it stay with an older version of the same code? Why?

You're cherry-picking one individual change to StitchMarker, out of very many changes that get committed to WTF all the time. It's cluttering up this commit; we wouldn't be able to read our changelogs if every edit to WTF required a corresponding edit to StitchMarker. If you want to change StitchMarker, please do it in a separate patch, but even then, I don't see much value, since performance tests are surely not intended to be run with asan.

[Alicia Boya García]

Created attachment 343500 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 343500 [details]
Patch

Clearing flags on attachment: 343500

Committed r233157: <https://trac.webkit.org/changeset/233157>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41435155>