[SanitizerCoverage] Drop !associated on metadata sections

[MaskRay]

Summary

In SanitizerCoverage, the metadata sections (__sancov_guards,
__sancov_cntrs, __sancov_bools) are referenced by functions. After
inlining, such a __sancov_* section can be referenced by more than one
functions, but its sh_link still refers to the original function's section.
(Note: a SHF_LINK_ORDER section referenced by a section other than its linked-to
section violates the invariant.)

If the original function's section is discarded (e.g. LTO internalization +
ld.lld --gc-sections), ld.lld may report a sh_link points to discarded section error.

This above reasoning means that !associated is not appropriate to be called by
an inlinable function. Non-interposable functions are inline candidates, so we
have to drop !associated. A __sancov_pcs is not referenced by other sections
but is expected to parallel a metadata section, so we have to make sure the two
sections are retained or discarded at the same time. A section group does the
trick. (Note: we have a module ctor, so getUniqueModuleId guarantees to
return a non-empty string, and GetOrCreateFunctionComdat guarantees to return
non-null.)

For interposable functions, we could keep using !associated, but
LTO can change the linkage to internal and allow such functions to be inlinable,
so we have to drop !associated, too. To not interfere with section
group resolution, we need to use the noduplicates variant (section group flag 0).
(This allows us to get rid of the ModuleID parameter.)
In -fno-pie and -fpie code (mostly dso_local), instrumented interposable
functions have WeakAny/LinkOnceAny linkages, which are rare. So the
section group header overload should be low.

This patch does not change the object file output for COFF (where !associated is ignored).

[vitalybuka]

Thanks for the quick fix!

llvm/lib/Transforms/Instrumentation/Instrumentation.cpp

81–82

this comment does not match condition now

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

686–688

Why removing these lines does not change COFF behaviour ?

[MaskRay]

Comments.

[MaskRay]

llvm/lib/Transforms/Instrumentation/Instrumentation.cpp

81–82

Thanks. BTW: we can delete the ModuleId block entirely.

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

686–688

I'll amend the description:

This patch does change the object file output for COFF (!associated is ignored).

[vitalybuka]

LGTM
If no more comment I'll accept tomorrow?

llvm/lib/Transforms/Instrumentation/Instrumentation.cpp

81–82

So it was added in D51902 and replaced with NoDuplicates for COFF in D54232

[morehouse]

llvm/test/Instrumentation/SanitizerCoverage/interposable-symbol-nocomdat.ll

1

This test now has comdats. Should we rename the test?

[MaskRay]

Precommit test improvement
Rename a test

[morehouse]

LGTM

[rnk]

What you have described is generally true, and it is why static locals cannot be part of the comdat group of an inline function (inline int f() { static int gv; return ++gv; }). Groups (and !associated, I guess, although I'm not super familiar) really only work for metadata applications, where the relocation edges go from the associated data towards the externally visible things. It is very difficult to construct a comdat group with more than one externally visible symbol. See all the confusion over the D5 comdat group.

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

686–688

I agree, it is redundant for COFF. The comdat group is what establishes the association links.

[ychen]

llvm/lib/Transforms/Instrumentation/Instrumentation.cpp

76

Internally, I saw this failure https://github.com/llvm/llvm-project/blob/a6a57f03be40cf2c546805155624a206649092ec/llvm/lib/Linker/LinkModules.cpp#L181 that may be caused by this for a test built with -fprofile-generate -flto for ELF. According to the title and description of the patch, that seems an unintended behavior change. Just check in to see if it is obvious how to fix this. Will provide the reproducer once it is available.

[MaskRay]

llvm/lib/Transforms/Instrumentation/Instrumentation.cpp

76

Is it called by the legacy LTO implementation LTOCodeGenerator::addModule? I guess new LTO does not exercise the code path.

I guess the fix is to ignore deduplication feature in ModuleLinker::run ComdatsChosen. comdat nodeduplicates was added to ELF this year.

llvm/include/llvm/Transforms/Instrumentation.h

 // Create a constant for Str so that we can pass it to the run-time lib.
 GlobalVariable *createPrivateGlobalForString(Module &M, StringRef Str,
                                              bool AllowMerging,
                                              const char *NamePrefix = "");
 
 // Returns F.getComdat() if it exists.
 // Otherwise creates a new comdat, sets F's comdat, and returns it.
 // Returns nullptr on failure.
-Comdat *GetOrCreateFunctionComdat(Function &F, Triple &T,
-                                  const std::string &ModuleId);
+Comdat *getOrCreateFunctionComdat(Function &F, Triple &T);
 
 // Insert GCOV profiling instrumentation
 struct GCOVOptions {
   static GCOVOptions getDefault();
 
   // Specify whether to emit .gcno files.
   bool EmitNotes;
 

llvm/lib/Transforms/IPO/SampleProfileProbe.cpp

void SampleProfileProber::instrumentOneFunc(Function &F, TargetMachine *TM) {
   // original module. The pseudo probes inserted into an imported functions
   // above will naturally not be emitted since the imported function is free
   // from object emission. However they will be emitted together with the
   // inliner functions that the imported function is inlined into. We are not
   // creating a comdat group for an import function since it's useless anyway.
   if (!F.isDeclarationForLinker()) {
     if (TM) {
       auto Triple = TM->getTargetTriple();
-      if (Triple.supportsCOMDAT() && TM->getFunctionSections()) {
-        GetOrCreateFunctionComdat(F, Triple, CurModuleUniqueId);
-      }
+      if (Triple.supportsCOMDAT() && TM->getFunctionSections())
+        getOrCreateFunctionComdat(F, Triple);
     }
   }
 }
 
 PreservedAnalyses SampleProfileProbePass::run(Module &M,
                                               ModuleAnalysisManager &AM) {
   auto ModuleId = getUniqueModuleId(&M);
   // Create the pseudo probe desc metadata beforehand.

llvm/lib/Transforms/Instrumentation/Instrumentation.cpp

GlobalVariable *GV =
                          GlobalValue::PrivateLinkage, StrConst, NamePrefix);
   if (AllowMerging)
     GV->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
   GV->setAlignment(Align(1)); // Strings may not be merged w/o setting
                               // alignment explicitly.
   return GV;
 }
 
-Comdat *llvm::GetOrCreateFunctionComdat(Function &F, Triple &T,
-                                        const std::string &ModuleId) {
+Comdat *llvm::getOrCreateFunctionComdat(Function &F, Triple &T) {
   if (auto Comdat = F.getComdat()) return Comdat;
   assert(F.hasName());
   Module *M = F.getParent();
-  std::string Name = std::string(F.getName());
-
-  // Make a unique comdat name for internal linkage things on ELF. On COFF, the
-  // name of the comdat group identifies the leader symbol of the comdat group.
-  // The linkage of the leader symbol is considered during comdat resolution,
-  // and internal symbols with the same name from different objects will not be
-  // merged.
-  if (T.isOSBinFormatELF() && F.hasLocalLinkage()) {
-    if (ModuleId.empty())
-      return nullptr;
-    Name += ModuleId;
-  }
 
   // Make a new comdat for the function. Use the "no duplicates" selection kind
-  // for non-weak symbols if the object file format supports it.
-  Comdat *C = M->getOrInsertComdat(Name);
-  if (T.isOSBinFormatCOFF() && !F.isWeakForLinker())
+  // if the object file format supports it. For COFF we restrict it to non-weak
+  // symbols.
+  Comdat *C = M->getOrInsertComdat(F.getName());
+  if (T.isOSBinFormatELF() || (T.isOSBinFormatCOFF() && !F.isWeakForLinker()))
     C->setSelectionKind(Comdat::NoDuplicates);
   F.setComdat(C);
   return C;
 }
 
 /// initializeInstrumentation - Initialize all passes in the TransformUtils
 /// library.
 void llvm::initializeInstrumentation(PassRegistry &Registry) {

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

 
 GlobalVariable *ModuleSanitizerCoverage::CreateFunctionLocalArrayInSection(
     size_t NumElements, Function &F, Type *Ty, const char *Section) {
   ArrayType *ArrayTy = ArrayType::get(Ty, NumElements);
   auto Array = new GlobalVariable(
       *CurModule, ArrayTy, false, GlobalVariable::PrivateLinkage,
       Constant::getNullValue(ArrayTy), "__sancov_gen_");
 
-  if (TargetTriple.supportsCOMDAT() && !F.isInterposable())
-    if (auto Comdat =
-            GetOrCreateFunctionComdat(F, TargetTriple, CurModuleUniqueId))
+  if (TargetTriple.supportsCOMDAT() &&
+      (TargetTriple.isOSBinFormatELF() || !F.isInterposable()))
+    if (auto Comdat = getOrCreateFunctionComdat(F, TargetTriple))
       Array->setComdat(Comdat);
   Array->setSection(getSectionName(Section));
   Array->setAlignment(Align(DL->getTypeStoreSize(Ty).getFixedSize()));
   GlobalsToAppendToUsed.push_back(Array);
   GlobalsToAppendToCompilerUsed.push_back(Array);
-  MDNode *MD = MDNode::get(F.getContext(), ValueAsMetadata::get(&F));
-  Array->addMetadata(LLVMContext::MD_associated, *MD);
 
   return Array;
 }
 
 GlobalVariable *
 ModuleSanitizerCoverage::CreatePCArray(Function &F,
                                        ArrayRef<BasicBlock *> AllBlocks) {
   size_t N = AllBlocks.size();

llvm/test/Instrumentation/SanitizerCoverage/inline-bool-flag.ll

 ; Test -sanitizer-coverage-inline-bool-flag=1
 ; RUN: opt < %s -sancov -sanitizer-coverage-level=1 -sanitizer-coverage-inline-bool-flag=1  -S -enable-new-pm=0 | FileCheck %s
 ; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=1 -sanitizer-coverage-inline-bool-flag=1  -S | FileCheck %s
 
-; CHECK: @__sancov_gen_ = private global [1 x i1] zeroinitializer, section "__sancov_bools", comdat($foo), align 1, !associated !0
+; CHECK: $foo = comdat noduplicates
+; CHECK: @__sancov_gen_ = private global [1 x i1] zeroinitializer, section "__sancov_bools", comdat($foo), align 1{{$}}
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
 target triple = "x86_64-unknown-linux-gnu"
 define void @foo() {
 ; CHECK-LABEL: @foo(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TMP0:%.*]] = load i1, i1* getelementptr inbounds ([1 x i1], [1 x i1]* @__sancov_gen_, i64 0, i64 0), align 1, !nosanitize ![[#EMPTY:]]
 ; CHECK-NEXT:    [[TMP1:%.*]] = icmp eq i1 [[TMP0]], false
 ; CHECK-NEXT:    br i1 [[TMP1]], label [[TMP2:%.*]], label [[TMP3:%.*]]
 ; CHECK:       2:
 ; CHECK-NEXT:    store i1 true, i1* getelementptr inbounds ([1 x i1], [1 x i1]* @__sancov_gen_, i64 0, i64 0), align 1, !nosanitize ![[#EMPTY:]]
 ; CHECK-NEXT:    br label [[TMP3]]
 ; CHECK:       3:
 ; CHECK-NEXT:    ret void
 ;
 entry:
   ret void
 }
 ; CHECK: call void @__sanitizer_cov_bool_flag_init(i1* @__start___sancov_bools, i1* @__stop___sancov_bools)
 
-; CHECK: !0 = !{void ()* @foo}
 ; CHECK: ![[#EMPTY]] = !{}

llvm/test/Instrumentation/SanitizerCoverage/interposable-symbol-nocomdat.ll

 ; Test that interposable symbols do not get put in comdats.
-; RUN: opt < %s -sancov -sanitizer-coverage-level=3 -sanitizer-coverage-trace-pc-guard -mtriple x86_64-linux-gnu -S -enable-new-pm=0 | FileCheck %s
-; RUN: opt < %s -sancov -sanitizer-coverage-level=3 -sanitizer-coverage-trace-pc-guard -mtriple x86_64-windows-msvc -S -enable-new-pm=0 | FileCheck %s
-; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=3 -sanitizer-coverage-trace-pc-guard -mtriple x86_64-linux-gnu -S | FileCheck %s
-; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=3 -sanitizer-coverage-trace-pc-guard -mtriple x86_64-windows-msvc -S | FileCheck %s
+; RUN: opt < %s -sancov -sanitizer-coverage-level=3 -sanitizer-coverage-trace-pc-guard -mtriple x86_64-linux-gnu -S -enable-new-pm=0 | FileCheck %s --check-prefixes=CHECK,ELF
+; RUN: opt < %s -sancov -sanitizer-coverage-level=3 -sanitizer-coverage-trace-pc-guard -mtriple x86_64-windows-msvc -S -enable-new-pm=0 | FileCheck %s --check-prefixes=CHECK,COFF
+; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=3 -sanitizer-coverage-trace-pc-guard -mtriple x86_64-linux-gnu -S | FileCheck %s --check-prefixes=CHECK,ELF
+; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=3 -sanitizer-coverage-trace-pc-guard -mtriple x86_64-windows-msvc -S | FileCheck %s --check-prefixes=CHECK,COFF
 
 define void @Vanilla() {
 entry:
   ret void
 }
 
 define linkonce void @LinkOnce() {
 entry:
entry:
   ret void
 }
 
 define weak_odr void @WeakOdr() {
 entry:
   ret void
 }
 
-; CHECK:      @__sancov_gen_ = private global [1 x i32] zeroinitializer, section {{.*}}, comdat($Vanilla), align 4, !associated
-; CHECK-NEXT: @__sancov_gen_.1 = private global [1 x i32] zeroinitializer, section {{.*}}, align 4, !associated
-; CHECK-NEXT: @__sancov_gen_.2 = private global [1 x i32] zeroinitializer, section {{.*}}, align 4, !associated
-; CHECK-NEXT: @__sancov_gen_.3 = private global [1 x i32] zeroinitializer, section {{.*}}, comdat($LinkOnceOdr), align 4, !associated
-; CHECK-NEXT: @__sancov_gen_.4 = private global [1 x i32] zeroinitializer, section {{.*}}, comdat($WeakOdr), align 4, !associated
+; CHECK:      $Vanilla = comdat noduplicates
+; ELF:        $LinkOnceOdr = comdat noduplicates
+; COFF:       $LinkOnceOdr = comdat any
+; 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: define void @Vanilla() comdat {
-; CHECK: define linkonce void @LinkOnce() {
-; CHECK: define weak void @Weak() {
+; ELF:   define linkonce void @LinkOnce() comdat {
+; ELF:   define weak void @Weak() comdat {
+; COFF:  define linkonce void @LinkOnce() {
+; COFF:  define weak void @Weak() {
 ; CHECK: declare extern_weak void @ExternWeak()
 ; CHECK: define linkonce_odr void @LinkOnceOdr() comdat {
 ; CHECK: define weak_odr void @WeakOdr() comdat {

llvm/test/Instrumentation/SanitizerCoverage/trace-pc-guard-comdat.ll

 ; RUN: opt < %s -sancov -sanitizer-coverage-level=4 -sanitizer-coverage-trace-pc-guard  -S -enable-new-pm=0 | FileCheck %s --check-prefix=CHECK_TRACE_PC_GUARD
 ; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=4 -sanitizer-coverage-trace-pc-guard  -S | FileCheck %s --check-prefix=CHECK_TRACE_PC_GUARD
 
+; CHECK:      $foo = comdat noduplicates
+; CHECK:      $CallViaVptr = comdat noduplicates
 ; CHECK:      @__sancov_gen_ = private global [1 x i32] zeroinitializer, section "__sancov_guards", comdat($foo), align 4{{$}}
 ; CHECK-NEXT: @__sancov_gen_.1 = private global [1 x i32] zeroinitializer, section "__sancov_guards", comdat($CallViaVptr), align 4{{$}}
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
 target triple = "x86_64-unknown-linux-gnu"
 define void @foo(i32* %a) sanitize_address {
 entry:
   %tobool = icmp eq i32* %a, null