Currently ASAN_ENABLED only returns true in Clang, since it uses Clang-specific detection features.
Created attachment 343408 [details] Patch
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?
(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))
Created attachment 343429 [details] Patch
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
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?
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
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
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
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
(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.
Created attachment 343500 [details] Patch
Comment on attachment 343500 [details] Patch Clearing flags on attachment: 343500 Committed r233157: <https://trac.webkit.org/changeset/233157>
All reviewed patches have been landed. Closing bug.
<rdar://problem/41435155>