Bug 117762 – Provide optional OTHER_CFLAGS, OTHER_CPPFLAGS, OTHER_LDFLAGS additions for building with ASAN

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Farler]

To support nightly ASAN builds, we should do one of the following:

Option 1: New xcconfig.

Provide a new build configuration ASAN.xcconfig to append to OTHER_CFLAGS, OTHER_CPPFLAGS, and OTHER_LDFLAGS, inheriting from Base or DebugRelease.

You would pass -configuration ASAN to xcodebuild -- an asan target would need to be added to the makefiles.


Option 2: Edit existing xcconfigs.

Create something like OTHER_CFLAGS_ASAN_ENABLED_$(ASAN_ENABLED), OTHER_CPPFLAGS_ASAN_ENABLED_$(ASAN_ENABLED), and OTHER_LDFLAGS_ASAN_ENABLED_$(ASAN_ENABLED), include them in the base FLAGS variables, and then add:

OTHER_CFLAGS_ASAN_ENABLED_1 = -fsanitize=address -O1 -mllvm -asan-blacklist=$(BASE_DIR)/ignore.txt -Wno-error -fno-omit-frame-pointer -g -DUSE_SYSTEM_MALLOC=1;
OTHER_CPPLAGS_ASAN_ENABLED_1 = -fsanitize=address -O1 -mllvm -asan-blacklist=$(BASE_DIR)/ignore.txt -Wno-error -fno-omit-frame-pointer -g -DUSE_SYSTEM_MALLOC=1;
OTHER_LDFLAGS_ASAN_ENABLED_1 = -fsanitize=address;

You would pass ENABLE_ASAN=1 to xcodebuild, or ARGS="ENABLE_ASAN=1" to the makefiles

* Note: static libraries must not add -fsanitize=address to LDFLAGS.

[Mark Rowe (bdash)]

Note that the former option is adding a new configuration, which is not the same as adding a new .xcconfig file. Adding a new configuration is a relatively heavyweight approach so I think we should avoid it. Customizing configuration settings when ASAN is enabled seems preferable.

[David Farler]

To minimize changes to the xcconfigs, I'd like to drop an asan.xcconfig in Tools/asan. The only changes necessary to the DebugRelease.xconfigs etc is the conditionalized OTHER_LDFLAGS to allow the static library targets to ignore those flags (unfortunately libtool doesn't ignore unknown options and -f is already taken to mean something else). Hopefully we can get separate Xcode flags for use with ld and libtool separately in the future.

[David Farler]

Created attachment 206678 [details]
Patch

[Mark Rowe (bdash)]

Comment on attachment 206678 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:11099
> -		5A91469E8E9F8485C37A2876 /* JSSVGGraphicsElement.cpp */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.cpp.cpp; path = JSSVGGraphicsElement.cpp; sourceTree = "<group>"; };
> -		950C4C02BED8936F818E2F99 /* JSSVGGraphicsElement.h */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.c.h; path = JSSVGGraphicsElement.h; sourceTree = "<group>"; };
>  		B2FA3CB60AB75A6E000E5AC4 /* JSSVGImageElement.cpp */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.cpp.cpp; path = JSSVGImageElement.cpp; sourceTree = "<group>"; };

This seems unrelated to the overall purpose of this patch.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:27249
> +				OTHER_CFLAGS = "";
> +				OTHER_LDFLAGS = "$(OTHER_LDFLAGS_ENABLE_ASAN_$(ENABLE_ASAN))";

I don't think it's useful to have WebCoreExportFileGenerator use ASAN. It's only used during the build process.

> Tools/WebKitLauncher/Configurations/Base.xcconfig:40
> +OTHER_LDFLAGS_ENABLE_ASAN_YES = -fsanitize=address -lclang_rt.asan_osx_dynamic;

Why is this OTHER_LDFLAGS_ENABLE_ASAN_YES value different than every other value?

> Tools/asan/asan.xcconfig:8
> +OTHER_CFLAGS = -fsanitize=address -O1 -mllvm -asan-blacklist=$(ASAN_IGNORE) -Wno-error -fno-omit-frame-pointer -g -DUSE_SYSTEM_MALLOC=1;
> +OTHER_CPLUSPLUSFLAGS = -fsanitize=address -O1 -mllvm -asan-blacklist=$(ASAN_IGNORE) -Wno-error -fno-omit-frame-pointer -g -DUSE_SYSTEM_MALLOC=1;

It'd make sense to pull the value here out in to a setting that OTHER_CFLAGS / OTHER_CPLUSPLUSFLAGS are initialized to, something like ASAN_OTHER_CFLAGS.

Doesn't using -xcconfig for this cause these values to override those defined at the project and target levels? Isn't $(inherit) needed to allow those to show through?

> Tools/asan/asan.xcconfig:9
> +OTHER_LDFLAGS_ASAN = -fsanitize=address;

This doesn't appear to be used anywhere.

> Tools/asan/asan.xcconfig:19
> +DEVELOPER_APPLICATIONS_DIR = $(BASE_DIR)/Xcode.app/Contents/Developer/Applications
> +DEVELOPER_BIN_DIR = $(BASE_DIR)/Xcode.app/Contents/Developer/usr/bin
> +DEVELOPER_DIR = $(BASE_DIR)/Xcode.app/Contents/Developer
> +DEVELOPER_FRAMEWORKS_DIR = $(BASE_DIR)/Xcode.app/Contents/Developer/Library/Frameworks
> +DEVELOPER_FRAMEWORKS_DIR_QUOTED = "$(BASE_DIR)/Xcode.app/Contents/Developer/Library/Frameworks"
> +DEVELOPER_LIBRARY_DIR = $(BASE_DIR)/Xcode.app/Contents/Developer/Library
> +DEVELOPER_SDK_DIR = $(BASE_DIR)/Xcode.app/Contents/Developer/SDKs
> +DEVELOPER_TOOLS_DIR = $(BASE_DIR)/Xcode.app/Contents/Developer/Tools
> +DEVELOPER_USR_DIR = $(BASE_DIR)/Xcode.app/Contents/Developer/usr

It's not clear why any of this is necessary. These variables all derive their values from DEVELOPER_DIR, so the act of invoking a different copy of xcodebuild should result in them pointing to the correct copy of Xcode.app.

[David Farler]

(In reply to comment #4)
> (From update of attachment 206678 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206678&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:11099
> > -		5A91469E8E9F8485C37A2876 /* JSSVGGraphicsElement.cpp */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.cpp.cpp; path = JSSVGGraphicsElement.cpp; sourceTree = "<group>"; };
> > -		950C4C02BED8936F818E2F99 /* JSSVGGraphicsElement.h */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.c.h; path = JSSVGGraphicsElement.h; sourceTree = "<group>"; };
> >  		B2FA3CB60AB75A6E000E5AC4 /* JSSVGImageElement.cpp */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.cpp.cpp; path = JSSVGImageElement.cpp; sourceTree = "<group>"; };
> 
> This seems unrelated to the overall purpose of this patch.
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:27249
> > +				OTHER_CFLAGS = "";
> > +				OTHER_LDFLAGS = "$(OTHER_LDFLAGS_ENABLE_ASAN_$(ENABLE_ASAN))";
> 
> I don't think it's useful to have WebCoreExportFileGenerator use ASAN. It's only used during the build process.
> 
> > Tools/WebKitLauncher/Configurations/Base.xcconfig:40
> > +OTHER_LDFLAGS_ENABLE_ASAN_YES = -fsanitize=address -lclang_rt.asan_osx_dynamic;
> 
> Why is this OTHER_LDFLAGS_ENABLE_ASAN_YES value different than every other value?
> 
> > Tools/asan/asan.xcconfig:8
> > +OTHER_CFLAGS = -fsanitize=address -O1 -mllvm -asan-blacklist=$(ASAN_IGNORE) -Wno-error -fno-omit-frame-pointer -g -DUSE_SYSTEM_MALLOC=1;
> > +OTHER_CPLUSPLUSFLAGS = -fsanitize=address -O1 -mllvm -asan-blacklist=$(ASAN_IGNORE) -Wno-error -fno-omit-frame-pointer -g -DUSE_SYSTEM_MALLOC=1;
> 
> It'd make sense to pull the value here out in to a setting that OTHER_CFLAGS / OTHER_CPLUSPLUSFLAGS are initialized to, something like ASAN_OTHER_CFLAGS.

Sounds good.

> 
> Doesn't using -xcconfig for this cause these values to override those defined at the project and target levels? Isn't $(inherit) needed to allow those to show through?

Hm, yes, I think so, although no project was setting OTHER_C*FLAGS except for gtest and WebCore that I could see. Still, it would make sense to anticipate changes in the future.

> 
> > Tools/asan/asan.xcconfig:9
> > +OTHER_LDFLAGS_ASAN = -fsanitize=address;
> 
> This doesn't appear to be used anywhere.

I think I was trying something where I didn’t have to do the whole “OTHER_LDFLAGS_ASAN_ENABLED_YES”. I might like to go this route along with the ASAN_OTHER_CFLAGS and ASAN_OTHER_CPLUSPLUS_FLAGS if it works; it seems a little cleaner.

> 
> > Tools/asan/asan.xcconfig:19
> > +DEVELOPER_APPLICATIONS_DIR = $(BASE_DIR)/Xcode.app/Contents/Developer/Applications
> > +DEVELOPER_BIN_DIR = $(BASE_DIR)/Xcode.app/Contents/Developer/usr/bin
> > +DEVELOPER_DIR = $(BASE_DIR)/Xcode.app/Contents/Developer
> > +DEVELOPER_FRAMEWORKS_DIR = $(BASE_DIR)/Xcode.app/Contents/Developer/Library/Frameworks
> > +DEVELOPER_FRAMEWORKS_DIR_QUOTED = "$(BASE_DIR)/Xcode.app/Contents/Developer/Library/Frameworks"
> > +DEVELOPER_LIBRARY_DIR = $(BASE_DIR)/Xcode.app/Contents/Developer/Library
> > +DEVELOPER_SDK_DIR = $(BASE_DIR)/Xcode.app/Contents/Developer/SDKs
> > +DEVELOPER_TOOLS_DIR = $(BASE_DIR)/Xcode.app/Contents/Developer/Tools
> > +DEVELOPER_USR_DIR = $(BASE_DIR)/Xcode.app/Contents/Developer/usr
> 
> It's not clear why any of this is necessary. These variables all derive their values from DEVELOPER_DIR, so the act of invoking a different copy of xcodebuild should result in them pointing to the correct copy of Xcode.app.

This was inherited from the xcconfig that was given to me - I’ll try a build with this removed.

[Build Bot]

Comment on attachment 206678 [details]
Patch

Attachment 206678 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1086019

New failing tests:
http/tests/security/cross-origin-plugin-private-browsing-toggled.html
fullscreen/full-screen-iframe-with-max-width-height.html

[Build Bot]

Created attachment 206715 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[David Farler]

Created attachment 207350 [details]
Patch

[Mark Rowe (bdash)]

Comment on attachment 207350 [details]
Patch

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

Not yet marking as r+ due to my concern about Tools/TestWebKitAPI/Configurations/Base.xcconfig. Only needs trivial tweaks besides that.

> Tools/TestWebKitAPI/Configurations/Base.xcconfig:-46
> -OTHER_CPLUSPLUSFLAGS = $(OTHER_CPLUSPLUSFLAGS) -ftemplate-depth=256;

Is removing -ftemplate-depth=256 for production builds intentional?  That aspect isn't mentioned in your ChangeLog so I think it may be a mistake.

> Tools/asan/asan.xcconfig:35
> +// To use this config, add the following to your xcodebuild invocation:
> +// -xcconfig /path/to/asan.xcconfig
> +// ASAN_IGNORE=/path/to/asan/ignore/txtfile
> +//
> +// You must build with a version of clang that supports address
> +// sanitization and a libclang_rt.asan_osx_dynamic.dylib must be in your
> +// built product directory.
> +//
> +// To run WebKit with address sanitization on Mac OS X, the following
> +// environment variable are recommended be set:
> +// ASAN_OPTIONS="replace_intrin=0:abort_on_error=1:handle_segv=0"
> +//
> +// replace_intrin=0 tells ASan to not check memcpy, memmove, and similar
> +// functions. memcpy and memmove have the same address in Mac OS X libc
> +// and are implemented as memmove. ASan currently replaces "memcpy" with
> +// its own internal implementation which is plain memcpy, causing all
> +// calls to memmove to become memcpy on Mac OS X.
> +// See ASan: memcpy and memmove have the same address on Darwin
> +// http://llvm.org/bugs/show_bug.cgi?id=16362
> +//
> +// The current workaround for this bug is to hack compiler-rt, placing
> +// internal_memmove's implementation inside internal_memcpy, and having
> +// internal_memmove return a call to internal_memcpy. You will still
> +// want to have replace_intrin=0.
> +//
> +// abort_on_error=1 tells ASan to abort() instead of exit(1). This
> +// allows for Mac OS X applications spawned from launchd to generate a
> +// crash report and a readable stack trace.
> +//
> +// handle_segv=0 tells ASan to not catch SEGV and let the operating
> +// system handle it. On Mac OS X, this will trigger a proper crash
> +// report for applications spawned from launchd.
> +//
> +// For more information on AddressSanitizer flags, see
> +// https://code.google.com/p/address-sanitizer/wiki/Flags

Please put all of this info on a wiki page and just provide a link to it from here.  And note that it's OS X now, not Mac OS X.

> Tools/asan/asan.xcconfig:40
> +ASAN_OTHER_CFLAGS = -fsanitize=address -O1 -mllvm -asan-blacklist=$(ASAN_IGNORE) -Wno-error -fno-omit-frame-pointer -g -DUSE_SYSTEM_MALLOC=1;
> +ASAN_OTHER_CPLUSPLUSFLAGS = -fsanitize=address -O1 -mllvm -asan-blacklist=$(ASAN_IGNORE) -Wno-error -fno-omit-frame-pointer -g -DUSE_SYSTEM_MALLOC=1;

Are theses two values the same? If so can we just initialize 40ASAN_OTHER_CPLUSPLUSFLAGS to $(ASAN_OTHER_CFLAGS) rather than duplicating it?

[David Farler]

(In reply to comment #9)
> (From update of attachment 207350 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207350&action=review
> 
> Not yet marking as r+ due to my concern about Tools/TestWebKitAPI/Configurations/Base.xcconfig. Only needs trivial tweaks besides that.
> 
> > Tools/TestWebKitAPI/Configurations/Base.xcconfig:-46
> > -OTHER_CPLUSPLUSFLAGS = $(OTHER_CPLUSPLUSFLAGS) -ftemplate-depth=256;
> 
> Is removing -ftemplate-depth=256 for production builds intentional?  That aspect isn't mentioned in your ChangeLog so I think it may be a mistake.
> 

This was intentional. I moved this to DebugRelease to match the pattern in the rest of the files and I wasn’t able to inherit this setting since they were imported via #include. I didn’t see anywhere else where Base was used but, if there are other implications doing that, I can move all of the flags down to Base. What do you think?

> Please put all of this info on a wiki page and just provide a link to it from here.  And note that it's OS X now, not Mac OS X.

Will do.

> > Tools/asan/asan.xcconfig:40
> > +ASAN_OTHER_CFLAGS = -fsanitize=address -O1 -mllvm -asan-blacklist=$(ASAN_IGNORE) -Wno-error -fno-omit-frame-pointer -g -DUSE_SYSTEM_MALLOC=1;
> > +ASAN_OTHER_CPLUSPLUSFLAGS = -fsanitize=address -O1 -mllvm -asan-blacklist=$(ASAN_IGNORE) -Wno-error -fno-omit-frame-pointer -g -DUSE_SYSTEM_MALLOC=1;
> 
> Are theses two values the same? If so can we just initialize 40ASAN_OTHER_CPLUSPLUSFLAGS to $(ASAN_OTHER_CFLAGS) rather than duplicating it?

Yeah, I don’t see why not, as long as Xcode will honor it.

[Mark Rowe (bdash)]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 207350 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=207350&action=review
> > 
> > Not yet marking as r+ due to my concern about Tools/TestWebKitAPI/Configurations/Base.xcconfig. Only needs trivial tweaks besides that.
> > 
> > > Tools/TestWebKitAPI/Configurations/Base.xcconfig:-46
> > > -OTHER_CPLUSPLUSFLAGS = $(OTHER_CPLUSPLUSFLAGS) -ftemplate-depth=256;
> > 
> > Is removing -ftemplate-depth=256 for production builds intentional?  That aspect isn't mentioned in your ChangeLog so I think it may be a mistake.
> > 
> 
> This was intentional. I moved this to DebugRelease to match the pattern in the rest of the files and I wasn’t able to inherit this setting since they were imported via #include. I didn’t see anywhere else where Base was used but, if there are other implications doing that, I can move all of the flags down to Base. What do you think?

Base.xcconfig is used for Production, and is included by DebugRelease.xcconfig for Debug and Release configurations. DebugRelease.xcconfig is used only for the Debug and Release configurations.

[David Farler]

Created attachment 208152 [details]
Patch

[Mark Rowe (bdash)]

Comment on attachment 208152 [details]
Patch

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

> Tools/TestWebKitAPI/Configurations/DebugRelease.xcconfig:38
> +OTHER_CPLUSPLUSFLAGS = -ftemplate-depth=256 $(ASAN_OTHER_CPLUSPLUSFLAGS);

It's a little bit sad that we have to duplicate this here. I'm not sure there's an alternative approach that's substantially better though.

[David Farler]

Committed r153756: <http://trac.webkit.org/changeset/153756>