[asan] Avoid putting globals in a comdat section.

[pgousseau]

Summary

Putting globals in a comdat for dead-stripping changes the semantic and can potentially cause false negative odr violations at link time.
If odr indicators are used, we keep the comdat sections, as link time odr violations will be dectected for the odr indicator symbols.

This fixes PR 47925

[pgousseau]

fix lint issue

[pgousseau]

Ping

[pgousseau]

Ping

[vitalybuka]

I guess odr-indicator=1 is for reliable ODR violation detection and odr-indicator=0 of smaller binary size.

[pgousseau]

In D95859#2568106, @vitalybuka wrote:

I guess odr-indicator=1 is for reliable ODR violation detection and odr-indicator=0 of smaller binary size.

Thank you for the comment, yes, this change can potentially break the assumption that odr-indicator=0 reduces binary size, as it will prevent certain linker (eg bfd 2.30), to dead strip unused asan globals’ metadata. Do I understand your point correctly?

But I think correct semantic should take priority over binary size optimizations, please let me know your thoughts on this.

[vitalybuka]

In D95859#2568230, @pgousseau wrote:

In D95859#2568106, @vitalybuka wrote:

I guess odr-indicator=1 is for reliable ODR violation detection and odr-indicator=0 of smaller binary size.

Thank you for the comment, yes, this change can potentially break the assumption that odr-indicator=0 reduces binary size, as it will prevent certain linker (eg bfd 2.30), to dead strip unused asan globals’ metadata. Do I understand your point correctly?

But I think correct semantic should take priority over binary size optimizations, please let me know your thoughts on this.

Sanitizers don't mind false-negatives if it can noticeably help with some performance metrics. But I can't reproduce expected size regression.

[vitalybuka]

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

2093

do we need the same for others?

[pgousseau]

In D95859#2575618, @vitalybuka wrote:

In D95859#2568230, @pgousseau wrote:

In D95859#2568106, @vitalybuka wrote:

I guess odr-indicator=1 is for reliable ODR violation detection and odr-indicator=0 of smaller binary size.

Thank you for the comment, yes, this change can potentially break the assumption that odr-indicator=0 reduces binary size, as it will prevent certain linker (eg bfd 2.30), to dead strip unused asan globals’ metadata. Do I understand your point correctly?

But I think correct semantic should take priority over binary size optimizations, please let me know your thoughts on this.

Sanitizers don't mind false-negatives if it can noticeably help with some performance metrics. But I can't reproduce expected size regression.

Using the SingleSource test suite, I found 15 tests out of 281 had a size regression. The size increase was at most 0.2%, is this an acceptable regression?
Using gnu ld and clang options below:

-O3 -fomit-frame-pointer -DNDEBUG -fsanitize=address -fno-sanitize=leak -fdata-sections -fno-common -fsanitize-address-globals-dead-stripping

Tests: 281
Metric: size

Program                                        results.ld.old results.ld.new diff
 test-suite...nchmarks/Stanford/FloatMM.test    4249440        4256232        0.2%
 test-suite...enchmarks/Stanford/Queens.test    4249240        4252216        0.1%
 test-suite...e/Benchmarks/Misc/flops-4.test    4248192        4251080        0.1%
 test-suite.../Benchmarks/Stanford/Perm.test    4245368        4248152        0.1%
 test-suite...enchmarks/Stanford/RealMM.test    4249440        4252136        0.1%
 test-suite...Benchmarks/Stanford/IntMM.test    4253536        4256232        0.1%
 test-suite...hmarks/Stanford/Quicksort.test    4245448        4248128        0.1%
 test-suite...marks/Stanford/Bubblesort.test    4245504        4248096        0.1%
 test-suite...enchmarks/Stanford/Towers.test    4250184        4252776        0.1%
 test-suite...chmarks/Stanford/Treesort.test    4245600        4248184        0.1%
 test-suite...Benchmarks/Stanford/Oscar.test    4253864        4256392        0.1%
 test-suite...enchmarks/Stanford/Puzzle.test    4254240        4256560        0.1%
 test-suite...rce/Benchmarks/Misc/flops.test    4259048        4260688        0.0%
 test-suite...arks/Misc-C++/oopack_v1p8.test    4326088        4326504        0.0%
 test-suite...ms_struct-bitfield-init-1.test    4241904        4241992        0.0%

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

2093

In the COFF case, I found that setting COMDAT for global metadata does not hide ODR violations at link time, so I did not touch the COFF case.
I also checked that COMDAT was necessary for the MSVC linker to perform dead stripping.

[vitalybuka]

Thanks for benchmarks.
LGTM
@eugenis ?

[eugenis]

I think this is acceptable, LGTM.

[pgousseau]

Thank you vitalybuka eugenis for reviewing! I have pushed the change at https://reviews.llvm.org/rG27830bc2b1b8
Sorry I forgot to add the reviewers and phabricator url to the commit message -_-

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

for (size_t i = 0; i < ExtendedGlobals.size(); i++) {
     // The MSVC linker always inserts padding when linking incrementally. We
     // cope with that by aligning each struct to its size, which must be a power
     // of two.
     unsigned SizeOfGlobalStruct = DL.getTypeAllocSize(Initializer->getType());
     assert(isPowerOf2_32(SizeOfGlobalStruct) &&
            "global metadata will not be padded appropriately");
     Metadata->setAlignment(assumeAligned(SizeOfGlobalStruct));
 
     SetComdatForGlobalMetadata(G, Metadata, "");
   }
 
   // Update llvm.compiler.used, adding the new metadata globals. This is
   // needed so that during LTO these variables stay alive.
   if (!MetadataGlobals.empty())
     appendToCompilerUsed(M, MetadataGlobals);
 }
 
 void ModuleAddressSanitizer::InstrumentGlobalsELF(
     IRBuilder<> &IRB, Module &M, ArrayRef<GlobalVariable *> ExtendedGlobals,
     ArrayRef<Constant *> MetadataInitializers,
     const std::string &UniqueModuleId) {
   assert(ExtendedGlobals.size() == MetadataInitializers.size());
 
+  // Putting globals in a comdat changes the semantic and potentially cause
+  // false negative odr violations at link time. If odr indicators are used, we
+  // keep the comdat sections, as link time odr violations will be dectected on
+  // the odr indicator symbols.
+  bool UseComdatForGlobalsGC = UseOdrIndicator;
+
   SmallVector<GlobalValue *, 16> MetadataGlobals(ExtendedGlobals.size());
   for (size_t i = 0; i < ExtendedGlobals.size(); i++) {
     GlobalVariable *G = ExtendedGlobals[i];
     GlobalVariable *Metadata =
         CreateMetadataGlobal(M, MetadataInitializers[i], G->getName());
     MDNode *MD = MDNode::get(M.getContext(), ValueAsMetadata::get(G));
     Metadata->setMetadata(LLVMContext::MD_associated, MD);
     MetadataGlobals[i] = Metadata;
 
+    if (UseComdatForGlobalsGC)
       SetComdatForGlobalMetadata(G, Metadata, UniqueModuleId);
   }
 
   // Update llvm.compiler.used, adding the new metadata globals. This is
   // needed so that during LTO these variables stay alive.
   if (!MetadataGlobals.empty())
     appendToCompilerUsed(M, MetadataGlobals);
 
   // RegisteredFlag serves two purposes. First, we can pass it to dladdr()

llvm/test/Instrumentation/AddressSanitizer/global_metadata.ll

 @dyn_init_global = global i32 0, align 4
 @blacklisted_global = global i32 0, align 4
 @_ZZ4funcvE10static_var = internal global i32 0, align 4
 @.str = private unnamed_addr constant [14 x i8] c"Hello, world!\00", align 1
 @llvm.global_ctors = appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @_GLOBAL__sub_I_asan_globals.cpp, i8* null }]
 
 ; Check that globals were instrumented:
 
-; CHECK: @global = global { i32, [60 x i8] } zeroinitializer, comdat, align 32
-; CHECK: @.str = internal constant { [14 x i8], [50 x i8] } { [14 x i8] c"Hello, world!\00", [50 x i8] zeroinitializer }, comdat($".str${{[01-9a-f]+}}"), align 32
+; CHECK: @global = global { i32, [60 x i8] } zeroinitializer, align 32
+; CHECK: @.str = internal constant { [14 x i8], [50 x i8] } { [14 x i8] c"Hello, world!\00", [50 x i8] zeroinitializer }, align 32
 
 ; Check emitted location descriptions:
 ; CHECK: [[VARNAME:@___asan_gen_.[0-9]+]] = private unnamed_addr constant [7 x i8] c"global\00", align 1
 ; CHECK: [[FILENAME:@___asan_gen_.[0-9]+]] = private unnamed_addr constant [22 x i8] c"/tmp/asan-globals.cpp\00", align 1
 ; CHECK: [[LOCDESCR:@___asan_gen_.[0-9]+]] = private unnamed_addr constant { [22 x i8]*, i32, i32 } { [22 x i8]* [[FILENAME]], i32 5, i32 5 }
 ; NOALIAS: @__asan_global_global = {{.*}}i64 ptrtoint ({ i32, [60 x i8] }* @global to i64){{.*}} section "asan_globals"{{.*}}, !associated
 ; NOALIAS: @__asan_global_.str = {{.*}}i64 ptrtoint ({ [14 x i8], [50 x i8] }* @{{.str|1}} to i64){{.*}} section "asan_globals"{{.*}}, !associated
 ; ALIAS: @__asan_global_global = {{.*}}i64 ptrtoint ({ i32, [60 x i8] }* @0 to i64){{.*}} section "asan_globals"{{.*}}, !associated

llvm/test/Instrumentation/AddressSanitizer/global_with_comdat.ll

+; Check that we disable the use of comdat sections for the garbage collection
+; of globals.
+; This is to avoid false negative ODR violations detection at link time.
+; We keep using comdats for garbage collection if odr indicators are
+; enabled as indicator symbols will cause link time odr violations.
+; This is to fix PR 47925.
+;
+; RUN: opt < %s -asan -asan-module -enable-new-pm=0 -asan-globals-live-support=1 -S | FileCheck %s --check-prefixes=CHECK,NOCOMDAT
+; RUN: opt < %s -passes='asan-pipeline'             -asan-globals-live-support=1 -S | FileCheck %s --check-prefixes=CHECK,NOCOMDAT
+; Check that enabling odr indicators enables comdat for globals.
+; RUN: opt < %s -asan -asan-module -enable-new-pm=0 -asan-globals-live-support=1 -asan-use-odr-indicator=1 -S | FileCheck %s --check-prefixes=CHECK,COMDAT
+; RUN: opt < %s -passes='asan-pipeline'             -asan-globals-live-support=1 -asan-use-odr-indicator=1 -S | FileCheck %s --check-prefixes=CHECK,COMDAT
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Globals:
+@global = global i32 0, align 4
+@dyn_init_global = global i32 0, align 4
+@blacklisted_global = global i32 0, align 4
+@_ZZ4funcvE10static_var = internal global i32 0, align 4
+@.str = private unnamed_addr constant [14 x i8] c"Hello, world!\00", align 1
[e-mail]_ctors = appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @_GLOBAL__sub_I_asan_globals.cpp, i8* null }]
+
+; Check that globals were instrumented:
+
+; COMDAT: $global = comdat any
+; COMDAT: $dyn_init_global = comdat any
+; COMDAT: $"_ZZ4funcvE10static_var${{[01-9a-f]+}}" = comdat any
+; COMDAT: $".str${{[01-9a-f]+}}" = comdat any
+
+; NOCOMDAT-NOT: $global = comdat any
+; NOCOMDAT-NOT: $dyn_init_global = comdat any
+; NOCOMDAT-NOT: $"_ZZ4funcvE10static_var${{[01-9a-f]+}}" = comdat any
+; NOCOMDAT-NOT: $".str${{[01-9a-f]+}}" = comdat any
+
+; COMDAT: @global = global { i32, [60 x i8] } zeroinitializer, comdat, align 32
+; COMDAT: @dyn_init_global = global { i32, [60 x i8] } zeroinitializer, comdat, align 32
+; COMDAT: @_ZZ4funcvE10static_var = internal global { i32, [60 x i8] } zeroinitializer, comdat($"_ZZ4funcvE10static_var${{[01-9a-f]+}}"), align 32
+; COMDAT: @.str = internal constant { [14 x i8], [50 x i8] } { [14 x i8] c"Hello, world!\00", [50 x i8] zeroinitializer }, comdat($".str${{[01-9a-f]+}}"), align 32
+
+; NOCOMDAT: @global = global { i32, [60 x i8] } zeroinitializer, align 32
+; NOCOMDAT: @dyn_init_global = global { i32, [60 x i8] } zeroinitializer, align 32
+; NOCOMDAT: @_ZZ4funcvE10static_var = internal global { i32, [60 x i8] } zeroinitializer, align 32
+; NOCOMDAT: @.str = internal constant { [14 x i8], [50 x i8] } { [14 x i8] c"Hello, world!\00", [50 x i8] zeroinitializer }, align 32
+
+; Check emitted location descriptions:
+; CHECK: [[VARNAME:@___asan_gen_.[0-9]+]] = private unnamed_addr constant [7 x i8] c"global\00", align 1
+; CHECK: [[FILENAME:@___asan_gen_.[0-9]+]] = private unnamed_addr constant [22 x i8] c"/tmp/asan-globals.cpp\00", align 1
+; CHECK: [[LOCDESCR:@___asan_gen_.[0-9]+]] = private unnamed_addr constant { [22 x i8]*, i32, i32 } { [22 x i8]* [[FILENAME]], i32 5, i32 5 }
+; COMDAT: @__asan_global_global = {{.*}}i64 ptrtoint ({ i32, [60 x i8] }* @global to i64){{.*}} section "asan_globals"{{.*}}, !associated
+; COMDAT: @__asan_global_.str = {{.*}}i64 ptrtoint ({ [14 x i8], [50 x i8] }* @{{.str|1}} to i64){{.*}} section "asan_globals"{{.*}}, !associated
+
+; The metadata has to be inserted to llvm.compiler.used to avoid being stripped
+; during LTO.
+; CHECK: @llvm.compiler.used {{.*}} @__asan_global_global {{.*}} section "llvm.metadata"
+
+; Check that location descriptors and global names were passed into __asan_register_globals:
+; CHECK: call void @__asan_register_elf_globals(i64 ptrtoint (i64* @___asan_globals_registered to i64), i64 ptrtoint (i64* @__start_asan_globals to i64), i64 ptrtoint (i64* @__stop_asan_globals to i64))
+
+; Function Attrs: nounwind sanitize_address
+define internal void @__cxx_global_var_init() #0 section ".text.startup" {
+entry:
+  %0 = load i32, i32* @global, align 4
+  store i32 %0, i32* @dyn_init_global, align 4
+  ret void
+}
+
+; Function Attrs: nounwind sanitize_address
+define void @_Z4funcv() #1 {
+entry:
+  %literal = alloca i8*, align 8
+  store i8* getelementptr inbounds ([14 x i8], [14 x i8]* @.str, i32 0, i32 0), i8** %literal, align 8
+  ret void
+}
+
+; Function Attrs: nounwind sanitize_address
+define internal void @_GLOBAL__sub_I_asan_globals.cpp() #0 section ".text.startup" {
+entry:
+  call void @__cxx_global_var_init()
+  ret void
+}
+
+attributes #0 = { nounwind sanitize_address }
+attributes #1 = { nounwind sanitize_address "less-precise-fpmad"="false" "frame-pointer"="none" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-realign-stack" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
+!llvm.asan.globals = !{!0, !1, !2, !3, !4}
+!llvm.ident = !{!5}
+
+!0 = !{i32* @global, !6, !"global", i1 false, i1 false}
+!1 = !{i32* @dyn_init_global, !7, !"dyn_init_global", i1 true, i1 false}
+!2 = !{i32* @blacklisted_global, null, null, i1 false, i1 true}
+!3 = !{i32* @_ZZ4funcvE10static_var, !8, !"static_var", i1 false, i1 false}
+!4 = !{[14 x i8]* @.str, !9, !"<string literal>", i1 false, i1 false}
+
+!5 = !{!"clang version 3.5.0 (211282)"}
+
+!6 = !{!"/tmp/asan-globals.cpp", i32 5, i32 5}
+!7 = !{!"/tmp/asan-globals.cpp", i32 7, i32 5}
+!8 = !{!"/tmp/asan-globals.cpp", i32 12, i32 14}
+!9 = !{!"/tmp/asan-globals.cpp", i32 14, i32 25}

llvm/test/Instrumentation/AddressSanitizer/instrument-section-invalid-c-ident.ll

 ; This test checks that sections with names not resembling valid C identifiers
 ; are instrumented.
 ; RUN: opt < %s -asan -asan-module -S | FileCheck %s
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-netbsd"
 
 module asm ".hidden invalid$c$name"
 
 @data1 = dso_local global i32 1, align 4
 @data2 = dso_local global i32 2, align 4
 @__invalid$c$name_sym_data1 = internal constant i8* bitcast (i32* @data1 to i8*), section "invalid$c$name", align 8
 @__invalid$c$name_sym_data2 = internal constant i8* bitcast (i32* @data2 to i8*), section "invalid$c$name", align 8
-; CHECK: @"__invalid$c$name_sym_data1" = internal constant{{.*}}, section "invalid$c$name", comdat
-; CHECK-NEXT: @"__invalid$c$name_sym_data2" = internal constant{{.*}}, section "invalid$c$name", comdat
+; CHECK: @"__invalid$c$name_sym_data1" = internal constant{{.*}}, section "invalid$c$name"
+; CHECK-NEXT: @"__invalid$c$name_sym_data2" = internal constant{{.*}}, section "invalid$c$name"
 ; CHECK: @"__asan_global___invalid$c$name_sym_data1"
 ; CHECK-NEXT: @"__asan_global___invalid$c$name_sym_data2"