[Driver] Default -fsanitize-address-globals-dead-stripping to true for ELF

[MaskRay]

Summary

-fsanitize-address-globals-dead-stripping is the default for non-ELF
platforms. For ELF, we disabled it to work around an ancient gold 2.26
bug. However, some platforms (Fuchsia and PS) default the option to
true.

This patch changes -fsanitize-address-globals-dead-stripping to true for all ELF
platforms. Without specifying -fdata-sections (non-default for most ELF
platforms), asan_globals can only be GCed if the monolithic .data/.bss section
is GCed, which makes it less effective.
However, I think this simplified rule is better than making the
-fsanitize-address-globals-dead-stripping default dependent on another option.

Close https://github.com/llvm/llvm-project/issues/63127

[phosek]

LGTM

[rnk]

I think there's a fair bit more cleanup and simplification to be done, see asanUsesGlobalsGC and the comments there. We could check CGOpts.DataSections right there, for example, and rip out the whole cc1 option. Feel free to approach it incrementally, this makes sense to me policy wise.

[MaskRay]

In D152604#4415392, @rnk wrote:

I think there's a fair bit more cleanup and simplification to be done, see asanUsesGlobalsGC and the comments there. We could check CGOpts.DataSections right there, for example, and rip out the whole cc1 option. Feel free to approach it incrementally, this makes sense to me policy wise.

Thanks for mentioning this piece of code. It seems that I forgot to update this comment in D120394 that allowed -fno-data-sections -fsanitize-address-globals-dead-stripping.

I have thought about not applying -fsanitize-address-globals-dead-stripping in -fno-data-sections mode, so that we won't have sections like .bss.a, but the driver complexity/cognitive load is high, so I think let's just simplify things and make -fsanitize-address-globals-dead-stripping default to true. (PS defaults to -fdata-sections, but Fuchsia doesn't.)

[eugenis]

LGTM

This is long overdue, I think. Thank you!

[rnk]

In D152604#4415403, @MaskRay wrote:

I have thought about not applying -fsanitize-address-globals-dead-stripping in -fno-data-sections mode, so that we won't have sections like .bss.a, but the driver complexity/cognitive load is high, so I think let's just simplify things and make -fsanitize-address-globals-dead-stripping default to true. (PS defaults to -fdata-sections, but Fuchsia doesn't.)

I'd like to get further simplifications by always emitting globals into the asan_globals section on ELF (whether the global data is sliced and associated or not) and always making the module ctor comdat, so there is only one initializer per DSO. I think there is cognitive overhead to the way that ELF objects only *sometimes* use the fancy comdat scheme if they provide a unique module signature, but fallback on the portable global registration scheme when a unique signature cannot be computed because all global symbols have local linkage. I'll look into sending a patch.

[glandium]

In the realm of unintended consequences, this broke ODR violation detection when linking a rust static library with asan enabled because, while __asan_globals_registered is COMDAT in clang, for some reason, it's not in rust... So you end up with two asan.module_ctor that call __asan_register_elf_globals, each with their own __asan_globals_registered, but both using the same range of globals, so all globals are registered twice. (That's probably a bug in the rust compiler. I thought I'd mention this here in case someone follows the same path as I did. Edit: it is: https://github.com/rust-lang/rust/issues/113404)

[rnk]

It sounds like two users have hit this now: Chromium and Rust folks. This is a flag flip, so it's pretty small and safe to rollback, and IMO we should consider that while we debug the underlying issue.

[glandium]

In D152604#4494975, @rnk wrote:

It sounds like two users have hit this now: Chromium and Rust folks.

s/Rust/Firefox/. And it looks like we're hitting it for the same reason: linking a static rust (LTOed) library with C++ code.

[MaskRay]

In D152604#4494975, @rnk wrote:

It sounds like two users have hit this now: Chromium and Rust folks. This is a flag flip, so it's pretty small and safe to rollback, and IMO we should consider that while we debug the underlying issue.

Chromium's https://crbug.com/1459233 is also a Rust issue. All the evidence so far has shown that there is some issue with Rust or how Chromium and Firefox mix C++ and Rust, probably due to a special use case of LTO+asan.
I don't find justification to revert this Clang Driver change.
Chromium's https://crbug.com/1459233 seems to suggest that it has its own compiler-rt ODR violation issue and should be fixed there. Adding -fno-sanitize-address-globals-dead-stripping can be quite good workaround.

If we do identify a good reason to revert, we will consider reverting, but a downstream language implementation or its user does unsupported things which worked well and now broke due to a changed clang driver default does not seem a strong enough justification.

[justincady]

I suspect there's another unintended consequence of this change with regards to broken ODR violation detection. I filed an issue and am connecting it here should someone find this review first.

clang/lib/Driver/SanitizerArgs.cpp

AsanPoisonCustomArrayCookie = Args.hasFlag(
         options::OPT_fno_sanitize_address_poison_custom_array_cookie,
         AsanPoisonCustomArrayCookie);
 
     AsanOutlineInstrumentation =
         Args.hasFlag(options::OPT_fsanitize_address_outline_instrumentation,
                      options::OPT_fno_sanitize_address_outline_instrumentation,
                      AsanOutlineInstrumentation);
 
-    // As a workaround for a bug in gold 2.26 and earlier, dead stripping of
-    // globals in ASan is disabled by default on most ELF targets.
-    // See https://sourceware.org/bugzilla/show_bug.cgi?id=19002
     AsanGlobalsDeadStripping = Args.hasFlag(
         options::OPT_fsanitize_address_globals_dead_stripping,
-        options::OPT_fno_sanitize_address_globals_dead_stripping,
-        !TC.getTriple().isOSBinFormatELF() || TC.getTriple().isOSFuchsia() ||
-            TC.getTriple().isPS());
+        options::OPT_fno_sanitize_address_globals_dead_stripping, true);
 
     // Enable ODR indicators which allow better handling of mixed instrumented
     // and uninstrumented globals. Disable them for Windows where weak odr
     // indicators (.weak.__odr_asan_gen*) may cause multiple definition linker
     // errors in the absence of -lldmingw.
     AsanUseOdrIndicator =
         Args.hasFlag(options::OPT_fsanitize_address_use_odr_indicator,
                      options::OPT_fno_sanitize_address_use_odr_indicator,

clang/test/Driver/fsanitize.c

 
 // RUN: %clang --target=x86_64-linux-gnu -fsanitize=address -fsanitize-address-poison-custom-array-cookie -fno-sanitize-address-poison-custom-array-cookie %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE-BOTH-OFF
 // CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE-BOTH-OFF-NOT: -cc1{{.*}}address-poison-custom-array-cookie
 
 // RUN: %clang --target=x86_64-linux-gnu -fsanitize=address %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-WITHOUT-POISON-CUSTOM-ARRAY-NEW-COOKIE
 // CHECK-ASAN-WITHOUT-POISON-CUSTOM-ARRAY-NEW-COOKIE-NOT: -cc1{{.*}}address-poison-custom-array-cookie
 
 // RUN: %clang --target=x86_64-linux-gnu -fsanitize=address -fsanitize-address-globals-dead-stripping %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-GLOBALS
-// RUN: %clang --target=x86_64-linux-gnu -fsanitize=address %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-NO-ASAN-GLOBALS
+// RUN: %clang --target=x86_64-linux-gnu -fsanitize=address %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-GLOBALS
 // RUN: %clang --target=x86_64-linux-gnu -fsanitize=address -fsanitize-address-globals-dead-stripping -fno-sanitize-address-globals-dead-stripping %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-NO-ASAN-GLOBALS
 // RUN: %clang_cl --target=x86_64-windows-msvc -fsanitize=address -fsanitize-address-globals-dead-stripping -### -- %s 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-GLOBALS
 // RUN: %clang_cl --target=x86_64-windows-msvc -fsanitize=address -### -- %s 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-GLOBALS
 // RUN: %clang --target=x86_64-scei-ps4  -fsanitize=address %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-GLOBALS
 // RUN: %clang --target=x86_64-sie-ps5  -fsanitize=address %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-GLOBALS
 // CHECK-ASAN-GLOBALS: -cc1{{.*}}-fsanitize-address-globals-dead-stripping
 // CHECK-NO-ASAN-GLOBALS-NOT: -cc1{{.*}}-fsanitize-address-globals-dead-stripping