[MaskRay]

MergeFunctions::mergeTwoFunctions may turn identical functions into a
private unnamed function, which trips assert(F.hasName()). Without
assertions, all unnamed functions share an empty-name IR comdat (which
yields no ELF section group per MCContext::getELFSection).

Fix #228820: Just drop comdat for unnamed functions. SanitizerCoverage
then retains the metadata sections via llvm.used. It's acceptable to
have redundat metadata sections with discarded associated text sections.

Improve comments at https://reviews.llvm.org/D97430 modified code.

[llvmorg-github-actions]

@llvm/pr-subscribers-llvm-transforms

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

Author: Fangrui Song (MaskRay)

Changes

MergeFunctions::mergeTwoFunctions may turn identical functions into a
private unnamed function, which trips assert(F.hasName()). Without
assertions, all unnamed functions share an empty-name IR comdat (which
yields no ELF section group per MCContext::getELFSection).

Fix #228820: Just drop comdat for unnamed functions. SanitizerCoverage
then retains the metadata sections via llvm.used. It's acceptable to
have redundat metadata sections with discarded associated text sections.

Improve comments at https://reviews.llvm.org/D97430 modified code.

---

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

3 Files Affected:

• (modified) llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp (+8-10)
• (modified) llvm/lib/Transforms/Utils/Instrumentation.cpp (+4-1)
• (modified) llvm/test/Instrumentation/SanitizerCoverage/interposable-symbol.ll (+20-7)

diff --git a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
index f2e2441c4b1bc..9e59f60d3d926 100644
--- a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
+++ b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
@@ -775,7 +775,12 @@ GlobalVariable *ModuleSanitizerCoverage::CreateFunctionLocalArrayInSection(
       *CurModule, ArrayTy, false, GlobalVariable::PrivateLinkage,
       Constant::getNullValue(ArrayTy), "__sancov_gen_");
 
-  // noipa doesn't affect linkage, so it is ignored when deciding on a comdat.
+  // sancov_pcs parallels the other arrays, so they must be retained or
+  // discarded together. Put them in F's comdat to tie them to F for linker GC
+  // benefit. Outside ELF (nodeduplicate), a new comdat for an interposable F
+  // could prevail over a strong definition (COFF: the weak external becomes a
+  // COMDAT definition; Wasm: comdats deduplicate first). noipa doesn't affect
+  // symbol resolution.
   if (TargetTriple.supportsCOMDAT() &&
       (F.hasComdat() || TargetTriple.isOSBinFormatELF() ||
        !F.isInterposable(/*CheckNoIPA=*/false)))
@@ -784,15 +789,8 @@ GlobalVariable *ModuleSanitizerCoverage::CreateFunctionLocalArrayInSection(
   Array->setSection(getSectionName(Section));
   Array->setAlignment(Align(DL->getTypeStoreSize(Ty).getFixedValue()));
 
-  // sancov_pcs parallels the other metadata section(s). Optimizers (e.g.
-  // GlobalOpt/ConstantMerge) may not discard sancov_pcs and the other
-  // section(s) as a unit, so we conservatively retain all unconditionally in
-  // the compiler.
-  //
-  // With comdat (COFF/ELF), the linker can guarantee the associated sections
-  // will be retained or discarded as a unit, so llvm.compiler.used is
-  // sufficient. Otherwise, conservatively make all of them retained by the
-  // linker.
+  // Optimizers (e.g. GlobalOpt/ConstantMerge) may not discard the arrays as a
+  // unit, so retain them in the compiler; without a comdat, in the linker too.
   if (Array->hasComdat())
     GlobalsToAppendToCompilerUsed.push_back(Array);
   else
diff --git a/llvm/lib/Transforms/Utils/Instrumentation.cpp b/llvm/lib/Transforms/Utils/Instrumentation.cpp
index 3e43b85a11a3c..5f71703725ba1 100644
--- a/llvm/lib/Transforms/Utils/Instrumentation.cpp
+++ b/llvm/lib/Transforms/Utils/Instrumentation.cpp
@@ -96,7 +96,10 @@ GlobalVariable *llvm::createPrivateGlobalForString(Module &M, StringRef Str,
 Comdat *llvm::getOrCreateFunctionComdat(Function &F, Triple &T) {
   if (auto Comdat = F.getComdat())
     return Comdat;
-  assert(F.hasName());
+  // Unnamed functions would share a single empty-name comdat, which has little
+  // linker garbage collection benefit.
+  if (!F.hasName())
+    return nullptr;
   Module *M = F.getParent();
 
   // Make a new comdat for the function. Use the "no duplicates" selection kind
diff --git a/llvm/test/Instrumentation/SanitizerCoverage/interposable-symbol.ll b/llvm/test/Instrumentation/SanitizerCoverage/interposable-symbol.ll
index 9131d4bf27dba..64b381c68fd59 100644
--- a/llvm/test/Instrumentation/SanitizerCoverage/interposable-symbol.ll
+++ b/llvm/test/Instrumentation/SanitizerCoverage/interposable-symbol.ll
@@ -7,6 +7,8 @@ $WeakComdat = comdat any
 
 define void @Vanilla() {
 entry:
+  call i32 @0()
+  call i32 @1()
   ret void
 }
 
@@ -43,19 +45,30 @@ entry:
   ret void
 }
 
+define private i32 @0() {
+  ret i32 0
+}
+
+define i32 @1() {
+  ret i32 1
+}
 
 ; CHECK:      $Vanilla = comdat nodeduplicate
 ; ELF:        $LinkOnceOdr = comdat nodeduplicate
 ; COFF:       $LinkOnceOdr = comdat any
 ; CHECK:      $WeakComdat = comdat any
 ; CHECK:      $NoIPA = comdat nodeduplicate
-; CHECK:      @__sancov_gen_ = private global [1 x i32] zeroinitializer, section {{.*}}, comdat($Vanilla), align 4{{$}}
-; CHECK-NEXT: @__sancov_gen_.1 = private global [1 x i32] zeroinitializer, section {{.*}}, align 4{{$}}
-; CHECK-NEXT: @__sancov_gen_.2 = private global [1 x i32] zeroinitializer, section {{.*}}, align 4{{$}}
-; CHECK-NEXT: @__sancov_gen_.3 = private global [1 x i32] zeroinitializer, section {{.*}}, comdat($LinkOnceOdr), align 4{{$}}
-; CHECK-NEXT: @__sancov_gen_.4 = private global [1 x i32] zeroinitializer, section {{.*}}, comdat($WeakOdr), align 4{{$}}
-; CHECK-NEXT: @__sancov_gen_.5 = private global [1 x i32] zeroinitializer, section {{.*}}, comdat($WeakComdat), align 4{{$}}
-; CHECK-NEXT: @__sancov_gen_.6 = private global [1 x i32] zeroinitializer, section {{.*}}, comdat($NoIPA), align 4{{$}}
+; CHECK:      @__sancov_gen_ = private global [1 x i32] zeroinitializer, section "{{[^"]*}}", comdat($Vanilla), align 4{{$}}
+; ELF-NEXT:   @__sancov_gen_.1 = private global [1 x i32] zeroinitializer, section "{{[^"]*}}", comdat($LinkOnce), align 4{{$}}
+; ELF-NEXT:   @__sancov_gen_.2 = private global [1 x i32] zeroinitializer, section "{{[^"]*}}", comdat($Weak), align 4{{$}}
+; COFF-NEXT:  @__sancov_gen_.1 = private global [1 x i32] zeroinitializer, section "{{[^"]*}}", align 4{{$}}
+; COFF-NEXT:  @__sancov_gen_.2 = private global [1 x i32] zeroinitializer, section "{{[^"]*}}", align 4{{$}}
+; CHECK-NEXT: @__sancov_gen_.3 = private global [1 x i32] zeroinitializer, section "{{[^"]*}}", comdat($LinkOnceOdr), align 4{{$}}
+; CHECK-NEXT: @__sancov_gen_.4 = private global [1 x i32] zeroinitializer, section "{{[^"]*}}", comdat($WeakOdr), align 4{{$}}
+; CHECK-NEXT: @__sancov_gen_.5 = private global [1 x i32] zeroinitializer, section "{{[^"]*}}", comdat($WeakComdat), align 4{{$}}
+; CHECK-NEXT: @__sancov_gen_.6 = private global [1 x i32] zeroinitializer, section "{{[^"]*}}", comdat($NoIPA), align 4{{$}}
+; CHECK-NEXT: @__sancov_gen_.7 = private global [1 x i32] zeroinitializer, section "{{[^"]*}}", align 4{{$}}
+; CHECK-NEXT: @__sancov_gen_.8 = private global [1 x i32] zeroinitializer, section "{{[^"]*}}", align 4{{$}}
 
 ; CHECK: define void @Vanilla() comdat {
 ; ELF:   define linkonce void @LinkOnce() comdat {

[vitalybuka]

Not the same, but but very similar #228866, consider referencing or co-author?
LGTM.

[MaskRay]

Not the same, but but very similar #228866, consider referencing or co-author? LGTM.

No , i don't take code or test from that heavily ai generated patch ,. Adding a co authored trailer would be reward low quality contribution