[asan] Ensure __asan_register_elf_globals is called in COMDAT asan.module_ctor

[MaskRay]

On ELF platforms, when there is no global variable and the unique module ID is
non-empty, COMDAT asan.module_ctor is created with no
__asan_register_elf_globals calls. If this COMDAT is the prevailing copy
selected by the linker, the linkage unit will have no
__asan_register_elf_globals call: the redzone will not be poisoned and ODR
violation checker will not work (#67677).

This behavior is benign for -fno-sanitize-address-globals-dead-stripping because
asan.module_ctor functions that call __asan_register_globals
(InstrumentGlobalsWithMetadataArray) do not use COMDAT.

To fix #67677:

• Use COMDAT for -fsanitize-address-globals-dead-stripping on ELF platforms.
• Call __asan_register_elf_globals even if there is no global variable.
• If the unique module ID is empty, don't call SetComdatForGlobalMetadata
  (placing @.str in a COMDAT would incorrectly discard internal @.str in
  other compile units)

Alternatively, when there is no global variable, asan.module_ctor is not COMDAT
and does not call __asan_register_elf_globals. However, the asan.module_ctor
function cannot be eliminated by the linker.

Tested the following script. Only ELF -fsanitize-address-globals-dead-stripping has changed behaviors.

echo > a.cc  # no global variable, empty uniqueModuleId
echo 'void f() {}' > b.cc  # with global variable, with uniqueModuleId
echo 'int g;' > c.cc  # with global variable
for t in x86_64-linux-gnu arm64-apple-macosx x86_64-windows-msvc; do
  for gc in -f{,no-}sanitize-address-globals-dead-stripping; do
    for f in a.cc b.cc c.cc; do
      echo /tmp/Rel/bin/clang -S --target=$t -fsanitize=address $gc $f -o -
      /tmp/Rel/bin/clang -S --target=$t -fsanitize=address $gc $f -o - | sed -n '/asan.module_ctor/,/ret/p'
    done
  done
done

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

@llvm/pr-subscribers-llvm-transforms

Changes

On ELF platforms, when there is no global variable, COMDAT asan.module_ctor is
created with no __asan_register_elf_globals calls. If this COMDAT is the
prevailing copy selected by the linker, the linkage unit will have no
__asan_register_elf_globals call: the redzone will not be poisoned and ODR
violation checker will not work (#67677).

This behavior is benign for -fno-sanitize-address-globals-dead-stripping because
asan.module_ctor functions that call __asan_register_globals
(InstrumentGlobalsWithMetadataArray) do not use COMDAT.

To fix #67677:

• Use COMDAT for -fsanitize-address-globals-dead-stripping on ELF platforms.
• Call __asan_register_elf_globals even if there is no global variable.

Alternatively, when there is no global variable, asan.module_ctor is not COMDAT
and does not call __asan_register_elf_globals. However, the asan.module_ctor
function cannot be eliminated by the linker.

Tested the following script. Only ELF -fsanitize-address-globals-dead-stripping has changed behaviors.

echo > a.cc  # no global variable, empty uniqueModuleId
echo 'void f() {}' > b.cc  # with global variable, with uniqueModuleId
echo 'int g;' > c.cc  # with global variable
for t in x86_64-linux-gnu arm64-apple-macosx x86_64-windows-msvc; do
  for gc in -f{,no-}sanitize-address-globals-dead-stripping; do
    for f in a.cc b.cc c.cc; do
      echo /tmp/Rel/bin/clang -S --target=$t -fsanitize=address $gc $f -o -
      /tmp/Rel/bin/clang -S --target=$t -fsanitize=address $gc $f -o - | sed -n '/asan.module_ctor/,/ret/p'
    done
  done
done

---

Full diff: https://github.com/llvm/llvm-project/pull/67745.diff

3 Files Affected:

• (modified) llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp (+25-22)
• (modified) llvm/test/Instrumentation/AddressSanitizer/basic.ll (+3-1)
• (modified) llvm/test/Instrumentation/AddressSanitizer/global_metadata_array.ll (+15-4)

diff --git a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
index bde5fba20f3b7a6..2c6070004816d0f 100644
--- a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -2237,7 +2237,7 @@ void ModuleAddressSanitizer::InstrumentGlobalsELF(
 
   // We also need to unregister globals at the end, e.g., when a shared library
   // gets closed.
-  if (DestructorKind != AsanDtorKind::None) {
+  if (DestructorKind != AsanDtorKind::None && !MetadataGlobals.empty()) {
     IRBuilder<> IrbDtor(CreateAsanModuleDtor(M));
     IrbDtor.CreateCall(AsanUnregisterElfGlobals,
                        {IRB.CreatePointerCast(RegisteredFlag, IntptrTy),
@@ -2345,8 +2345,6 @@ void ModuleAddressSanitizer::InstrumentGlobalsWithMetadataArray(
 // asan constructor is comdat-compatible.
 bool ModuleAddressSanitizer::InstrumentGlobals(IRBuilder<> &IRB, Module &M,
                                                bool *CtorComdat) {
-  *CtorComdat = false;
-
   // Build set of globals that are aliased by some GA, where
   // getExcludedAliasedGlobal(GA) returns the relevant GlobalVariable.
   SmallPtrSet<const GlobalVariable *, 16> AliasedGlobalExclusions;
@@ -2364,11 +2362,6 @@ bool ModuleAddressSanitizer::InstrumentGlobals(IRBuilder<> &IRB, Module &M,
   }
 
   size_t n = GlobalsToChange.size();
-  if (n == 0) {
-    *CtorComdat = true;
-    return false;
-  }
-
   auto &DL = M.getDataLayout();
 
   // A global is described by a structure
@@ -2391,8 +2384,11 @@ bool ModuleAddressSanitizer::InstrumentGlobals(IRBuilder<> &IRB, Module &M,
 
   // We shouldn't merge same module names, as this string serves as unique
   // module ID in runtime.
-  GlobalVariable *ModuleName = createPrivateGlobalForString(
-      M, M.getModuleIdentifier(), /*AllowMerging*/ false, kAsanGenPrefix);
+  GlobalVariable *ModuleName =
+      n != 0
+          ? createPrivateGlobalForString(M, M.getModuleIdentifier(),
+                                         /*AllowMerging*/ false, kAsanGenPrefix)
+          : nullptr;
 
   for (size_t i = 0; i < n; i++) {
     GlobalVariable *G = GlobalsToChange[i];
@@ -2517,19 +2513,26 @@ bool ModuleAddressSanitizer::InstrumentGlobals(IRBuilder<> &IRB, Module &M,
   }
   appendToCompilerUsed(M, ArrayRef<GlobalValue *>(GlobalsToAddToUsedList));
 
-  std::string ELFUniqueModuleId =
-      (UseGlobalsGC && TargetTriple.isOSBinFormatELF()) ? getUniqueModuleId(&M)
-                                                        : "";
-
-  if (!ELFUniqueModuleId.empty()) {
-    InstrumentGlobalsELF(IRB, M, NewGlobals, Initializers, ELFUniqueModuleId);
+  if (UseGlobalsGC && TargetTriple.isOSBinFormatELF()) {
+    // Use COMDAT and register globals even if n == 0 to ensure that (a) the
+    // linkage unit will only have one module constructor, and (b) the register
+    // function will be called. The module destructor is not created when n ==
+    // 0.
     *CtorComdat = true;
-  } else if (UseGlobalsGC && TargetTriple.isOSBinFormatCOFF()) {
-    InstrumentGlobalsCOFF(IRB, M, NewGlobals, Initializers);
-  } else if (UseGlobalsGC && ShouldUseMachOGlobalsSection()) {
-    InstrumentGlobalsMachO(IRB, M, NewGlobals, Initializers);
+    InstrumentGlobalsELF(IRB, M, NewGlobals, Initializers, getUniqueModuleId(&M));
+  } else if (n == 0) {
+    // When UseGlobalsGC is false, COMDAT can still be used if n == 0, because
+    // all compile units will have identical module constructor/destructor.
+    *CtorComdat = TargetTriple.isOSBinFormatELF();
   } else {
-    InstrumentGlobalsWithMetadataArray(IRB, M, NewGlobals, Initializers);
+    *CtorComdat = false;
+    if (UseGlobalsGC && TargetTriple.isOSBinFormatCOFF()) {
+      InstrumentGlobalsCOFF(IRB, M, NewGlobals, Initializers);
+    } else if (UseGlobalsGC && ShouldUseMachOGlobalsSection()) {
+      InstrumentGlobalsMachO(IRB, M, NewGlobals, Initializers);
+    } else {
+      InstrumentGlobalsWithMetadataArray(IRB, M, NewGlobals, Initializers);
+    }
   }
 
   // Create calls for poisoning before initializers run and unpoisoning after.
@@ -2537,7 +2540,7 @@ bool ModuleAddressSanitizer::InstrumentGlobals(IRBuilder<> &IRB, Module &M,
     createInitializerPoisonCalls(M, ModuleName);
 
   LLVM_DEBUG(dbgs() << M);
-  return true;
+  return n != 0;
 }
 
 uint64_t
diff --git a/llvm/test/Instrumentation/AddressSanitizer/basic.ll b/llvm/test/Instrumentation/AddressSanitizer/basic.ll
index 068d6d18cd45eba..f9c5b1439de2f9f 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/basic.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/basic.ll
@@ -210,8 +210,10 @@ define void @test_swifterror_3() sanitize_address {
 
 ;; ctor/dtor have the nounwind attribute. See uwtable.ll, they additionally have
 ;; the uwtable attribute with the module flag "uwtable".
-; CHECK: define internal void @asan.module_ctor() #[[#ATTR:]] {{(comdat )?}}{
+; CHECK: define internal void @asan.module_ctor() #[[#ATTR:]] comdat {
 ; CHECK: call void @__asan_init()
+;; __asan_register_elf_globals is called even if this module does not contain global variables.
+; CHECK: call void @__asan_register_elf_globals(i64 ptrtoint (ptr @___asan_globals_registered to i64), i64 ptrtoint (ptr @__start_asan_globals to i64), i64 ptrtoint (ptr @__stop_asan_globals to i64))
 
 ; CHECK: attributes #[[#ATTR]] = { nounwind }
 
diff --git a/llvm/test/Instrumentation/AddressSanitizer/global_metadata_array.ll b/llvm/test/Instrumentation/AddressSanitizer/global_metadata_array.ll
index 0e4d1f168d0163f..1617bf9b67aa7c5 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/global_metadata_array.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/global_metadata_array.ll
@@ -1,8 +1,12 @@
-; RUN: opt < %s -passes=asan -asan-globals-live-support=0 -mtriple=x86_64-unknown-linux-gnu -S | FileCheck --check-prefix=CHECK %s
-; RUN: opt < %s -passes=asan -asan-globals-live-support=0 -mtriple=x86_64-apple-macosx10.11.0 -S | FileCheck --check-prefix=CHECK %s
-; RUN: opt < %s -passes=asan -asan-globals-live-support=0 -mtriple=x86_64-pc-windows-msvc19.0.24215 -S | FileCheck --check-prefix=CHECK %s
-; RUN: opt < %s -passes=asan -asan-globals-live-support=0 -asan-mapping-scale=5 -mtriple=x86_64-unknown-linux-gnu -S | FileCheck --check-prefixes=CHECK,CHECK-S5 %s
+; RUN: rm -rf %t && split-file %s %t && cd %t
+; RUN: opt < a.ll -passes=asan -asan-globals-live-support=0 -mtriple=x86_64-unknown-linux-gnu -S | FileCheck --check-prefix=CHECK %s
+; RUN: opt < a.ll -passes=asan -asan-globals-live-support=0 -mtriple=x86_64-apple-macosx10.11.0 -S | FileCheck --check-prefix=CHECK %s
+; RUN: opt < a.ll -passes=asan -asan-globals-live-support=0 -mtriple=x86_64-pc-windows-msvc19.0.24215 -S | FileCheck --check-prefix=CHECK %s
+; RUN: opt < a.ll -passes=asan -asan-globals-live-support=0 -asan-mapping-scale=5 -mtriple=x86_64-unknown-linux-gnu -S | FileCheck --check-prefixes=CHECK,CHECK-S5 %s
 
+; RUN: opt < empty.ll -passes=asan -asan-globals-live-support=0 -mtriple=x86_64-unknown-linux-gnu -S | FileCheck --check-prefix=ELF-NOGC %s
+
+;--- a.ll
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 
 ; Globals:
@@ -59,3 +63,10 @@ attributes #1 = { nounwind sanitize_address "less-precise-fpmad"="false" "frame-
 !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}
+
+;; In the presence of instrumented global variables, asan.module_ctor do not use comdat.
+; CHECK: define internal void @asan.module_ctor() #[[#ATTR:]] {
+
+; ELF-NOGC: define internal void @asan.module_ctor() #[[#ATTR:]] comdat {
+
+;--- empty.ll

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[justincady]

LGTM. Thank you for investigating this!

I am not familiar with the code being changed, but I tested this diff in my environment (with the reproducer) and it detected the ODR violation appropriately.

It would be ideal to get this into an upcoming release of LLVM 17.0.X, if at all possible.

[MaskRay]

Merged manually in 1a4b9b6 to prevent GitHub truncating long lines in the description.

[vitalybuka]

https://lab.llvm.org/buildbot/#/builders/168 ?

[MaskRay]

lab.llvm.org/buildbot/#/builders/168 ?

Reverted. Edited the description about the issue:

If the unique module ID is empty, don't call SetComdatForGlobalMetadata (placing @.str in a COMDAT would incorrectly discard internal @.str in other compile units)

[MaskRay]

6420d33 is a wrong revert for an internal issue. I've commented on that one and will reland the same patch soon.