Bug 230100 – Fix ASan+UBSan builds: part two

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[David Kilzer (:ddkilzer)]

Fix ASan+UBSan builds: part two.

In Bug 229122 Comment #9, Alexey Proskuryakov writes:
> This is not what I was asking about. I was asking about including
> sanitizer.xcconfig twice. This happens because it's included by both
> asan.xcconfig and ubsan.xcconfig. There is a good chance that some settings
> are messed up (at least duplicated) because of this.

So it turns out that including sanitizer.xcconfig is basically okay (no duplicated settings), but investigating this lead me to discover something else:

Xcode variables defined in an xcconfig file that is specified on the command-line are not resolved until later, so two variables with the same name will overwrite each other (last one wins), even if they use '$(inherited)'.

This meant that, for example, defining WK_SANITIZER_OTHER_CFLAGS_YES in both sanitizer.xcconfig and ubsan.xcconfig will not work as expected--only one variable will win (the one in ubsan.xcconfig).

So this bug is to fix those issues and clean up the xcconfig files in Tools/sanitizer.

[Radar WebKit Bug Importer]

<rdar://problem/82926910>

[David Kilzer (:ddkilzer)]

Created attachment 437749 [details]
Patch v1

[Alexey Proskuryakov]

Comment on attachment 437749 [details]
Patch v1

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

> Tools/ChangeLog:16
> +          That mechanism doesn't work at the time when Xcode resolves
> +          variables from an xcconfig file specified on the command-line.

This is surprising, why would those be different? It seems like it cannot be by accident, someone would have to explicitly implement this behavior.

> Tools/sanitizer/sanitizer.xcconfig:7
> +OTHER_CFLAGS = $(inherited) -fno-omit-frame-pointer -g $(WK_UNDEFINED_BEHAVIOR_SANITIZER_OTHER_CFLAGS_$(WK_ENABLE_UNDEFINED_BEHAVIOR_SANITIZER));
> +OTHER_CPLUSPLUSFLAGS = $(inherited) $(WK_ADDRESS_SANITIZER_OTHER_CPLUSPLUSFLAGS_$(WK_ENABLE_ADDRESS_SANITIZER));

I think that OTHER_CFLAGS apply to C++ files when OTHER_CPLUSPLUSFLAGS isn't defined. But it is defined, so we need UBSan flags in the second line too.

> Tools/sanitizer/tsan.xcconfig:7
> +WK_ENABLE_THREAD_SANITIZER = $(ENABLE_THREAD_SANITIZER);

This looks like dead code, not used anywhere.

[David Kilzer (:ddkilzer)]

Comment on attachment 437749 [details]
Patch v1

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

>> Tools/ChangeLog:16
>> +          variables from an xcconfig file specified on the command-line.
> 
> This is surprising, why would those be different? It seems like it cannot be by accident, someone would have to explicitly implement this behavior.

The conclusion I reached is that the values can't be fully resolved when the xcconfig file(s) are parsed on the command line—they're just stored for later interpretation.

And the way the build system prints out the values early in each project build, it seems like they're stored as key-value pairs--not a block of text (such as pre-processed compiler output)--so only one value can be associated with one key.

I assume this is mostly a rhetorical question that doesn't block landing the patch since I'm just reporting the behavior I see.  I don't have any special insight into design decisions.

>> Tools/sanitizer/sanitizer.xcconfig:7
>> +OTHER_CPLUSPLUSFLAGS = $(inherited) $(WK_ADDRESS_SANITIZER_OTHER_CPLUSPLUSFLAGS_$(WK_ENABLE_ADDRESS_SANITIZER));
> 
> I think that OTHER_CFLAGS apply to C++ files when OTHER_CPLUSPLUSFLAGS isn't defined. But it is defined, so we need UBSan flags in the second line too.

Nope, that's not how it works.

Build settings from configuration file 'OpenSource/Tools/sanitizer/asan+ubsan.xcconfig':
    ENABLE_ADDRESS_SANITIZER = $(ENABLE_ADDRESS_SANITIZER_$(WK_ASAN_DISALLOWED))
    ENABLE_ADDRESS_SANITIZER_ = YES
    ENABLE_ADDRESS_SANITIZER_NO = YES
    ENABLE_UNDEFINED_BEHAVIOR_SANITIZER = $(ENABLE_UNDEFINED_BEHAVIOR_SANITIZER_$(WK_UBSAN_DISALLOWED))
    ENABLE_UNDEFINED_BEHAVIOR_SANITIZER_ = YES
    ENABLE_UNDEFINED_BEHAVIOR_SANITIZER_NO = YES
    GCC_OPTIMIZATION_LEVEL = $(GCC_OPTIMIZATION_LEVEL_$(CONFIGURATION))
    GCC_OPTIMIZATION_LEVEL_Debug = 0
    GCC_OPTIMIZATION_LEVEL_Production = 1
    GCC_OPTIMIZATION_LEVEL_Release = 1
    OTHER_CFLAGS = $(inherited) -fno-omit-frame-pointer -g $(WK_UNDEFINED_BEHAVIOR_SANITIZER_OTHER_CFLAGS_$(WK_ENABLE_UNDEFINED_BEHAVIOR_SANITIZER))
    OTHER_CPLUSPLUSFLAGS = $(inherited) $(WK_ADDRESS_SANITIZER_OTHER_CPLUSPLUSFLAGS_$(WK_ENABLE_ADDRESS_SANITIZER))
    OTHER_LDFLAGS = $(inherited) -Wl,-rpath,@executable_path/Frameworks
    WK_ADDRESS_SANITIZER_OTHER_CPLUSPLUSFLAGS_YES = -U_LIBCPP_HAS_NO_ASAN
    WK_ENABLE_ADDRESS_SANITIZER = $(ENABLE_ADDRESS_SANITIZER)
    WK_ENABLE_UNDEFINED_BEHAVIOR_SANITIZER = $(ENABLE_UNDEFINED_BEHAVIOR_SANITIZER)
    WK_UNDEFINED_BEHAVIOR_SANITIZER_OTHER_CFLAGS_YES = -fno-delete-null-pointer-checks -fno-optimize-sibling-calls -fno-sanitize=vptr

Both OTHER_CFLAGS and OTHER_CPLUSPLUSFLAGS are applied when building:

    /Applications/Xcode.app/Contents/Developer/Toolchains/OSX11.5.xctoolchain/usr/bin/clang -x c++ -target x86_64-apple-macos11.5 -fmessage-length=0 -fdiagnostics-show-note-include-stack -fmacro-backtrace-limit=0 -std=gnu++1z -stdlib=libc++ -Wno-trigraphs -fno-exceptions -fno-rtti -fno-sanitize=vptr -fpascal-strings -O1 -fno-common -Werror -Wno-missing-field-initializers -Wmissing-prototypes -Wunreachable-code -Wnon-virtual-dtor -Wno-overloaded-virtual -Wno-exit-time-destructors -Wno-missing-braces -Wparentheses -Wswitch -Wunused-function -Wno-unused-label -Wno-unused-parameter -Wunused-variable -Wunused-value -Wempty-body -Wuninitialized -Wno-unknown-pragmas -Wno-shadow -Wno-four-char-constants -Wno-conversion -Wconstant-conversion -Wint-conversion -Wbool-conversion -Wenum-conversion -Wno-float-conversion -Wnon-literal-null-conversion -Wobjc-literal-conversion -Wsign-compare -Wno-shorten-64-to-32 -Wnewline-eof -Wno-c++11-extensions -DNDEBUG -DPAS_BMALLOC -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.5.sdk -fasm-blocks -fstrict-aliasing -Wdeprecated-declarations -Winvalid-offsetof -g -fvisibility=hidden -fvisibility-inlines-hidden -fno-threadsafe-statics -Wno-sign-conversion -Winfinite-recursion -Wmove -Wcomma -Wblock-capture-autoreleasing -Wstrict-prototypes -Wrange-loop-analysis -Wno-semicolon-before-method-body -fsanitize=address -D_LIBCPP_HAS_NO_ASAN -fsanitize=undefined -fno-sanitize=enum,return,float-divide-by-zero,function,vptr -iquote WebKitBuild/Release/bmalloc.build/bmalloc-generated-files.hmap -IWebKitBuild/Release/bmalloc.build/bmalloc-own-target-headers.hmap -IWebKitBuild/Release/bmalloc.build/bmalloc-all-target-headers.hmap -iquote WebKitBuild/Release/bmalloc.build/bmalloc-project-headers.hmap -IWebKitBuild/Release/include -IWebKitBuild/Release/bmalloc.build/DerivedSources-normal/x86_64 -IWebKitBuild/Release/bmalloc.build/DerivedSources/x86_64 -IWebKitBuild/Release/bmalloc.build/DerivedSources -Wall -Wextra -Wcast-qual -Wchar-subscripts -Wconditional-uninitialized -Wextra-tokens -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wpacked -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wexit-time-destructors -Wglobal-constructors -Wtautological-compare -Wimplicit-fallthrough -Wvla -Wliteral-conversion -Wthread-safety -Wno-typedef-redefinition -FWebKitBuild/Release -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.5.sdk/System/Library/PrivateFrameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.5.sdk/System/Library/PrivateFrameworks -fno-omit-frame-pointer -g -fno-delete-null-pointer-checks -fno-optimize-sibling-calls -fno-sanitize=vptr -U_LIBCPP_HAS_NO_ASAN -MMD -MT dependencies -MF WebKitBuild/Release/bmalloc.build/Objects-normal-asan-ubsan/x86_64/Allocator.d --serialize-diagnostics WebKitBuild/Release/bmalloc.build/Objects-normal-asan-ubsan/x86_64/Allocator.dia -c OpenSource/Source/bmalloc/bmalloc/Allocator.cpp -o WebKitBuild/Release/bmalloc.build/Objects-normal-asan-ubsan/x86_64/Allocator.o

Easy way to test locally:

    $ make release ASAN=YES TSAN=NO UBSAN=YES

>> Tools/sanitizer/tsan.xcconfig:7
>> +WK_ENABLE_THREAD_SANITIZER = $(ENABLE_THREAD_SANITIZER);
> 
> This looks like dead code, not used anywhere.

I kept it for uniformity with the other *san.xcconfig files.

But I suppose it's obvious how to add it given the other examples, so I'll remove it.

Thanks for the review!

[David Kilzer (:ddkilzer)]

(In reply to Alexey Proskuryakov from comment #3)
> Comment on attachment 437749 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437749&action=review
> 
> > Tools/sanitizer/sanitizer.xcconfig:7
> > +OTHER_CFLAGS = $(inherited) -fno-omit-frame-pointer -g $(WK_UNDEFINED_BEHAVIOR_SANITIZER_OTHER_CFLAGS_$(WK_ENABLE_UNDEFINED_BEHAVIOR_SANITIZER));
> > +OTHER_CPLUSPLUSFLAGS = $(inherited) $(WK_ADDRESS_SANITIZER_OTHER_CPLUSPLUSFLAGS_$(WK_ENABLE_ADDRESS_SANITIZER));
> 
> I think that OTHER_CFLAGS apply to C++ files when OTHER_CPLUSPLUSFLAGS isn't
> defined. But it is defined, so we need UBSan flags in the second line too.

See also Bug 216746, which fixed this faulty assumption that lead to duplicate command-line switches:

    Enable AddressSanitizer in C++ std library templates
    <https://bugs.webkit.org/show_bug.cgi?id=216746>
    <https://trac.webkit.org/changeset/267358/webkit>

[David Kilzer (:ddkilzer)]

Created attachment 437913 [details]
Patch for landing

[EWS]

Committed r282297 (241570@main): <https://commits.webkit.org/241570@main>

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