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

[Justin Cady via Phabricator via cfe-commits]

justincady added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152604/new/

https://reviews.llvm.org/D152604

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152604/new/

https://reviews.llvm.org/D152604

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mike Hommey via Phabricator via cfe-commits]

glandium added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152604/new/

https://reviews.llvm.org/D152604

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152604/new/

https://reviews.llvm.org/D152604

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mike Hommey via Phabricator via cfe-commits]

glandium added a comment.

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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152604/new/

https://reviews.llvm.org/D152604

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152604/new/

https://reviews.llvm.org/D152604

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa8d3ae712290: [Driver] Default 
-fsanitize-address-globals-dead-stripping to true for ELF (authored by MaskRay).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152604/new/

https://reviews.llvm.org/D152604

Files:
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/fsanitize.c


Index: clang/test/Driver/fsanitize.c
===
--- clang/test/Driver/fsanitize.c
+++ clang/test/Driver/fsanitize.c
@@ -240,7 +240,7 @@
 // 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
Index: clang/lib/Driver/SanitizerArgs.cpp
===
--- clang/lib/Driver/SanitizerArgs.cpp
+++ clang/lib/Driver/SanitizerArgs.cpp
@@ -928,14 +928,9 @@
  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


Index: clang/test/Driver/fsanitize.c
===
--- clang/test/Driver/fsanitize.c
+++ clang/test/Driver/fsanitize.c
@@ -240,7 +240,7 @@
 // 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
Index: clang/lib/Driver/SanitizerArgs.cpp
===
--- clang/lib/Driver/SanitizerArgs.cpp
+++ clang/lib/Driver/SanitizerArgs.cpp
@@ -928,14 +928,9 @@
  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 

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

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis added a comment.

LGTM

This is long overdue, I think. Thank you!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152604/new/

https://reviews.llvm.org/D152604

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

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 we should make 
`-fsanitize-address-globals-dead-stripping` default to true. (PS defaults to 
`-fdata-sections`, but Fuchsia doesn't.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152604/new/

https://reviews.llvm.org/D152604

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152604/new/

https://reviews.llvm.org/D152604

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Petr Hosek via Phabricator via cfe-commits]

phosek accepted this revision.
phosek added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152604/new/

https://reviews.llvm.org/D152604

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay created this revision.
MaskRay added reviewers: Sanitizers, aeubanks, rnk, eugenis, phosek, probinson.
Herald added a project: All.
MaskRay requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

-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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152604

Files:
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/fsanitize.c


Index: clang/test/Driver/fsanitize.c
===
--- clang/test/Driver/fsanitize.c
+++ clang/test/Driver/fsanitize.c
@@ -240,7 +240,7 @@
 // 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
Index: clang/lib/Driver/SanitizerArgs.cpp
===
--- clang/lib/Driver/SanitizerArgs.cpp
+++ clang/lib/Driver/SanitizerArgs.cpp
@@ -928,14 +928,9 @@
  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


Index: clang/test/Driver/fsanitize.c
===
--- clang/test/Driver/fsanitize.c
+++ clang/test/Driver/fsanitize.c
@@ -240,7 +240,7 @@
 // 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
Index: clang/lib/Driver/SanitizerArgs.cpp
===
--- clang/lib/Driver/SanitizerArgs.cpp
+++ clang/lib/Driver/SanitizerArgs.cpp
@@ -928,14 +928,9 @@
  options::OPT_fno_sanitize_address_outline_instrumentation,
  AsanOutlineInstrumentation);
 
-