Bug 209963

Summary:

[Xcode] Replace ASAN_OTHER_CFLAGS and ASAN_OTHER_CPLUSPLUSFLAGS with $(inherited)

Product:

WebKit

Reporter:

David Kilzer (:ddkilzer) <ddkilzer>

Component:

Tools / Tests

Assignee:

David Kilzer (:ddkilzer) <ddkilzer>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, dino, eric.carlson, ews-watchlist, graouts, jbedard, jlewis3, keith_miller, kondapallykalyan, mark.lam, msaboff, ryanhaddad, saam, tsavell, tzagallo, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=206389
https://bugs.webkit.org/show_bug.cgi?id=206686
https://bugs.webkit.org/show_bug.cgi?id=210057

Attachments:

Description	Flags
Patch v1	none

Description David Kilzer (:ddkilzer) 2020-04-03 06:35:33 PDT

Replace ASAN_OTHER_CFLAGS and ASAN_OTHER_CPLUSPLUSFLAGS with the use of $(inherited) in Tools/asan/asan.xcconfig.

Comment 1 Radar WebKit Bug Importer 2020-04-03 06:36:51 PDT

<rdar://problem/61257504>

Comment 2 David Kilzer (:ddkilzer) 2020-04-03 06:52:53 PDT

BTW, TestWebKitAPI on macOS may not have been built with ASan enabled due to this:

OTHER_CFLAGS = $(ASAN_OTHER_CFLAGS) --system-header-prefix=WebKit/;
OTHER_CFLAGS[sdk=macosx*] = $(inherited) -iframework $(SDKROOT)$(SYSTEM_LIBRARY_DIR)/PrivateFrameworks;
OTHER_CPLUSPLUSFLAGS = $(ASAN_OTHER_CPLUSPLUSFLAGS);
OTHER_LDFLAGS = $(ASAN_OTHER_LDFLAGS);

So this change might surface some issues in macOS-only TestWebKitAPI tests.

Comment 3 David Kilzer (:ddkilzer) 2020-04-03 06:58:20 PDT

Created attachment 395372 [details]
Patch v1

Comment 4 EWS Watchlist 2020-04-03 06:59:05 PDT

Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE

Comment 5 Jonathan Bedard 2020-04-03 07:48:20 PDT

(In reply to David Kilzer (:ddkilzer) from comment #0)
> Replace ASAN_OTHER_CFLAGS and ASAN_OTHER_CPLUSPLUSFLAGS with the use of
> $(inherited) in Tools/asan/asan.xcconfig.

The change looks correct, how did you test this?

Comment 6 David Kilzer (:ddkilzer) 2020-04-03 09:31:50 PDT

(In reply to Jonathan Bedard from comment #5)
> (In reply to David Kilzer (:ddkilzer) from comment #0)
> > Replace ASAN_OTHER_CFLAGS and ASAN_OTHER_CPLUSPLUSFLAGS with the use of
> > $(inherited) in Tools/asan/asan.xcconfig.
> 
> The change looks correct, how did you test this?

Yep.  Built with ASan enabled on my 10.14.6 MacBook Pro:

$ make release SDKROOT=macosx.internal ARCHS=x86_64 ONLY_ACTIVE_ARCH=YES 2>&1 | tee ./build-release-macOS-asan.txt | ./Tools/Scripts/filter-build-webkit

$ grep -A2 '^CompileC' ./build-release-macOS-asan.txt | grep 'clang -x' | grep -v -- '-fsanitize=address' | grep -v 'Source/third_party/yasm/'
$ 

Everything except yasm (see Bug 190327 and r236898) was built with ASan.

Comment 7 Jonathan Bedard 2020-04-03 09:39:08 PDT

(In reply to David Kilzer (:ddkilzer) from comment #6)
> (In reply to Jonathan Bedard from comment #5)
> > (In reply to David Kilzer (:ddkilzer) from comment #0)
> > > Replace ASAN_OTHER_CFLAGS and ASAN_OTHER_CPLUSPLUSFLAGS with the use of
> > > $(inherited) in Tools/asan/asan.xcconfig.
> > 
> > The change looks correct, how did you test this?
> 
> Yep.  Built with ASan enabled on my 10.14.6 MacBook Pro:
> 
> $ make release SDKROOT=macosx.internal ARCHS=x86_64 ONLY_ACTIVE_ARCH=YES
> 2>&1 | tee ./build-release-macOS-asan.txt |
> ./Tools/Scripts/filter-build-webkit
> 
> $ grep -A2 '^CompileC' ./build-release-macOS-asan.txt | grep 'clang -x' |
> grep -v -- '-fsanitize=address' | grep -v 'Source/third_party/yasm/'
> $ 
> 
> Everything except yasm (see Bug 190327 and r236898) was built with ASan.

Probably should apply the r236898's logic to ImageDiff, but that's a different discussion.

Comment 8 EWS 2020-04-03 09:51:36 PDT

Committed r259466: <https://trac.webkit.org/changeset/259466>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395372 [details].

Comment 9 David Kilzer (:ddkilzer) 2020-04-03 09:55:44 PDT

(In reply to Jonathan Bedard from comment #7)
> (In reply to David Kilzer (:ddkilzer) from comment #6)
> > (In reply to Jonathan Bedard from comment #5)
> > > (In reply to David Kilzer (:ddkilzer) from comment #0)
> > > > Replace ASAN_OTHER_CFLAGS and ASAN_OTHER_CPLUSPLUSFLAGS with the use of
> > > > $(inherited) in Tools/asan/asan.xcconfig.
> > > 
> > > The change looks correct, how did you test this?
> > 
> > Yep.  Built with ASan enabled on my 10.14.6 MacBook Pro:
> > 
> > $ make release SDKROOT=macosx.internal ARCHS=x86_64 ONLY_ACTIVE_ARCH=YES
> > 2>&1 | tee ./build-release-macOS-asan.txt |
> > ./Tools/Scripts/filter-build-webkit
> > 
> > $ grep -A2 '^CompileC' ./build-release-macOS-asan.txt | grep 'clang -x' |
> > grep -v -- '-fsanitize=address' | grep -v 'Source/third_party/yasm/'
> > $ 
> > 
> > Everything except yasm (see Bug 190327 and r236898) was built with ASan.
> 
> Probably should apply the r236898's logic to ImageDiff, but that's a
> different discussion.

Why?  Does running ImageDiff with ASan slow down test runs significantly?

Comment 10 Jonathan Bedard 2020-04-03 10:10:57 PDT

(In reply to David Kilzer (:ddkilzer) from comment #9)
> (In reply to Jonathan Bedard from comment #7)
> > (In reply to David Kilzer (:ddkilzer) from comment #6)
> > > (In reply to Jonathan Bedard from comment #5)
> > > > (In reply to David Kilzer (:ddkilzer) from comment #0)
> > > > > Replace ASAN_OTHER_CFLAGS and ASAN_OTHER_CPLUSPLUSFLAGS with the use of
> > > > > $(inherited) in Tools/asan/asan.xcconfig.
> > > > 
> > > > The change looks correct, how did you test this?
> > > 
> > > Yep.  Built with ASan enabled on my 10.14.6 MacBook Pro:
> > > 
> > > $ make release SDKROOT=macosx.internal ARCHS=x86_64 ONLY_ACTIVE_ARCH=YES
> > > 2>&1 | tee ./build-release-macOS-asan.txt |
> > > ./Tools/Scripts/filter-build-webkit
> > > 
> > > $ grep -A2 '^CompileC' ./build-release-macOS-asan.txt | grep 'clang -x' |
> > > grep -v -- '-fsanitize=address' | grep -v 'Source/third_party/yasm/'
> > > $ 
> > > 
> > > Everything except yasm (see Bug 190327 and r236898) was built with ASan.
> > 
> > Probably should apply the r236898's logic to ImageDiff, but that's a
> > different discussion.
> 
> Why?  Does running ImageDiff with ASan slow down test runs significantly?

Not that I'm aware of, but like yasm, it's a tool built with something other than the SDK being tested.