[SanitizerCoverage] Use different module ctor names for trace-pc-guard and inline-8bit-counters

[MaskRay]

Summary

Fixes the main issue in PR41693

When both modes are used, two functions are created:
sancov.module_ctor, sancov.module_ctor.$LastUnique, where
$LastUnique is the current LastUnique counter that may be different in
another module.

sancov.module_ctor.$LastUnique belongs to the comdat group of the same
name (due to the non-null third field of the ctor in llvm.global_ctors).

COMDAT group section [    9] `.group' [sancov.module_ctor] contains 6 sections:
   [Index]    Name
   [   10]   .text.sancov.module_ctor
   [   11]   .rela.text.sancov.module_ctor
   [   12]   .text.sancov.module_ctor.6
   [   13]   .rela.text.sancov.module_ctor.6
   [   23]   .init_array.2
   [   24]   .rela.init_array.2

# 2 problems:
# 1) If sancov.module_ctor in this module is discarded, this group
# has a relocation to a discarded section. ld.bfd and gold will
# error. (Another issue: it is silently accepted by lld)
# 2) The comdat group has an unstable name that may be different in
# another translation unit. Even if the linker allows the dangling relocation
# (with --noinhibit-exec), there will be many undesired .init_array entries
COMDAT group section [   25] `.group' [sancov.module_ctor.6] contains 2 sections:
   [Index]    Name
   [   26]   .init_array.2
   [   27]   .rela.init_array.2

By using different module ctor names, the associated comdat group names
will also be different and thus stable across modules.

[MaskRay]

Add detailed description

[mcgrathr]

This works for my case that led to me to file https://bugs.llvm.org/show_bug.cgi?id=41693 as a linker issue.

[jakehehrlich]

Can you explain how this occurred a bit more and how this is solving the issue?

The second group (which has a distinct group name) references sections in the first group. Why does using two different unsuffixed names cause those new sections to be placed in the correct groups?

[mcgrathr]

Previously the same local symbol name was being used for both things in an uncoordinated fashion. The logic in places like ValueSymbolTable::createValueName leads to the second one getting renamed with some ".digits" suffix to make them unique.
Then the code that emits the references between the init_array and the actual function and sets the COMDAT group name assumed it could use the original name throughout, but in some places this was replaced with the suffixed name and in some places it was not, leading to the incorrect references.

[phosek]

LGTM but you may want to wait for @kcc @morehouse @vitalybuka to take a look as well since they're the owners.

[morehouse]

lib/Transforms/Instrumentation/SanitizerCoverage.cpp

289

Can we add an assert that the name of CtorFunc matches CtorName?

test/Instrumentation/SanitizerCoverage/trace-pc-guard-inline-8bit-counters.ll

8

Should we also CHECK-NOT for the suffix?

[MaskRay]

Apply morehouse's suggestions

test/Instrumentation/SanitizerCoverage/trace-pc-guard-inline-8bit-counters.ll

8

The .2 suffix is not predictable (well predictable for this small case as we know there is no other duplicate names but probably not good to check the negative pattern).

I'll change it to:

- ; CHECK: define internal void @sancov.module_ctor_trace_pc_guard() comdat
- ; CHECK: define internal void @sancov.module_ctor_8bit_counters() comdat
+ ; CHECK: define internal void @sancov.module_ctor_trace_pc_guard() comdat {
+ ; CHECK: define internal void @sancov.module_ctor_8bit_counters() comdat {

This ensure the comdat name doesn't take the form of comdat (sancov.module_ctor.2) that can cause us trouble.

[MaskRay]

Update the description to detail the 2 problems without this patch

lib/Transforms/Instrumentation/SanitizerCoverage.cpp

 static const char *const SanCovTraceConstCmp4 =
     "__sanitizer_cov_trace_const_cmp4";
 static const char *const SanCovTraceConstCmp8 =
     "__sanitizer_cov_trace_const_cmp8";
 static const char *const SanCovTraceDiv4 = "__sanitizer_cov_trace_div4";
 static const char *const SanCovTraceDiv8 = "__sanitizer_cov_trace_div8";
 static const char *const SanCovTraceGep = "__sanitizer_cov_trace_gep";
 static const char *const SanCovTraceSwitchName = "__sanitizer_cov_trace_switch";
-static const char *const SanCovModuleCtorName = "sancov.module_ctor";
+static const char *const SanCovModuleCtorTracePcGuardName =
+    "sancov.module_ctor_trace_pc_guard";
+static const char *const SanCovModuleCtor8bitCountersName =
+    "sancov.module_ctor_8bit_counters";
 static const uint64_t SanCtorAndDtorPriority = 2;
 
 static const char *const SanCovTracePCGuardName =
     "__sanitizer_cov_trace_pc_guard";
 static const char *const SanCovTracePCGuardInitName =
     "__sanitizer_cov_trace_pc_guard_init";
 static const char *const SanCov8bitCountersInitName =
     "__sanitizer_cov_8bit_counters_init";
bool InjectCoverage(Function &F, ArrayRef<BasicBlock *> AllBlocks,
                       bool IsLeafFunc = true);
   GlobalVariable *CreateFunctionLocalArrayInSection(size_t NumElements,
                                                     Function &F, Type *Ty,
                                                     const char *Section);
   GlobalVariable *CreatePCArray(Function &F, ArrayRef<BasicBlock *> AllBlocks);
   void CreateFunctionLocalArrays(Function &F, ArrayRef<BasicBlock *> AllBlocks);
   void InjectCoverageAtBlock(Function &F, BasicBlock &BB, size_t Idx,
                              bool IsLeafFunc = true);
-  Function *CreateInitCallsForSections(Module &M, const char *InitFunctionName,
-                                       Type *Ty, const char *Section);
+  Function *CreateInitCallsForSections(Module &M, const char *CtorName,
+                                       const char *InitFunctionName, Type *Ty,
+                                       const char *Section);
   std::pair<Value *, Value *> CreateSecStartEnd(Module &M, const char *Section,
                                                 Type *Ty);
 
   void SetNoSanitizeMetadata(Instruction *I) {
     I->setMetadata(I->getModule()->getMDKindID("nosanitize"),
                    MDNode::get(*C, None));
   }
 
SanitizerCoverageModule::CreateSecStartEnd(Module &M, const char *Section,
   // point to a uint64_t before the start of the array.
   auto SecStartI8Ptr = IRB.CreatePointerCast(SecStart, Int8PtrTy);
   auto GEP = IRB.CreateGEP(Int8Ty, SecStartI8Ptr,
                            ConstantInt::get(IntptrTy, sizeof(uint64_t)));
   return std::make_pair(IRB.CreatePointerCast(GEP, Ty), SecEndPtr);
 }
 
 Function *SanitizerCoverageModule::CreateInitCallsForSections(
-    Module &M, const char *InitFunctionName, Type *Ty,
+    Module &M, const char *CtorName, const char *InitFunctionName, Type *Ty,
     const char *Section) {
   auto SecStartEnd = CreateSecStartEnd(M, Section, Ty);
   auto SecStart = SecStartEnd.first;
   auto SecEnd = SecStartEnd.second;
   Function *CtorFunc;
   std::tie(CtorFunc, std::ignore) = createSanitizerCtorAndInitFunctions(
-      M, SanCovModuleCtorName, InitFunctionName, {Ty, Ty}, {SecStart, SecEnd});
+      M, CtorName, InitFunctionName, {Ty, Ty}, {SecStart, SecEnd});
 
   if (TargetTriple.supportsCOMDAT()) {
     // Use comdat to dedup CtorFunc.
-    CtorFunc->setComdat(M.getOrInsertComdat(SanCovModuleCtorName));
+    CtorFunc->setComdat(M.getOrInsertComdat(CtorName));
     appendToGlobalCtors(M, CtorFunc, SanCtorAndDtorPriority, CtorFunc);
   } else {
     appendToGlobalCtors(M, CtorFunc, SanCtorAndDtorPriority);
   }
 
   if (TargetTriple.isOSBinFormatCOFF()) {
     // In COFF files, if the contructors are set as COMDAT (they are because
     // COFF supports COMDAT) and the linker flag /OPT:REF (strip unreferenced
SanCovTracePCGuard =
       M.getOrInsertFunction(SanCovTracePCGuardName, VoidTy, Int32PtrTy);
 
   for (auto &F : M)
     runOnFunction(F);
 
   Function *Ctor = nullptr;
 
   if (FunctionGuardArray)
-    Ctor = CreateInitCallsForSections(M, SanCovTracePCGuardInitName, Int32PtrTy,
+    Ctor = CreateInitCallsForSections(M, SanCovModuleCtorTracePcGuardName,
+                                      SanCovTracePCGuardInitName, Int32PtrTy,
                                       SanCovGuardsSectionName);
   if (Function8bitCounterArray)
-    Ctor = CreateInitCallsForSections(M, SanCov8bitCountersInitName, Int8PtrTy,
+    Ctor = CreateInitCallsForSections(M, SanCovModuleCtor8bitCountersName,
+                                      SanCov8bitCountersInitName, Int8PtrTy,
                                       SanCovCountersSectionName);
   if (Ctor && Options.PCTable) {
     auto SecStartEnd = CreateSecStartEnd(M, SanCovPCsSectionName, IntptrPtrTy);
     FunctionCallee InitFunction = declareSanitizerInitFunction(
         M, SanCovPCsInitName, {IntptrPtrTy, IntptrPtrTy});
     IRBuilder<> IRBCtor(Ctor->getEntryBlock().getTerminator());
     IRBCtor.CreateCall(InitFunction, {SecStartEnd.first, SecStartEnd.second});
   }

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

 ; CHECK_TRACE_PC_GUARD: call void asm sideeffect "", ""()
 ; CHECK_TRACE_PC_GUARD: ret void
 
 ; CHECK_TRACE_PC_GUARD-LABEL: define void @CallViaVptr
 ; CHECK_TRACE_PC_GUARD: call void @__sanitizer_cov_trace_pc_indir
 ; CHECK_TRACE_PC_GUARD: call void @__sanitizer_cov_trace_pc_indir
 ; CHECK_TRACE_PC_GUARD: ret void
 
-; CHECK_TRACE_PC_GUARD-LABEL: define internal void @sancov.module_ctor() comdat
-
+; CHECK_TRACE_PC_GUARD-LABEL: define internal void @sancov.module_ctor_trace_pc_guard() comdat

test/Instrumentation/SanitizerCoverage/trace-pc-guard-inline-8bit-counters.ll

+; RUN: opt < %s -sancov -sanitizer-coverage-level=1 -sanitizer-coverage-trace-pc-guard -sanitizer-coverage-inline-8bit-counters -S | FileCheck %s
+
+; Module ctors should have stable names across modules, not something like
+; @sancov.module_ctor.3 that may cause duplicate ctors after linked together.
+
+; CHECK: define internal void @sancov.module_ctor_trace_pc_guard() comdat
+; CHECK: define internal void @sancov.module_ctor_8bit_counters() comdat
+
+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() {
+  ret void
+}

test/Instrumentation/SanitizerCoverage/trace-pc-guard-nocomdat.ll

 ; CHECK_TRACE_PC_GUARD: call void asm sideeffect "", ""()
 ; CHECK_TRACE_PC_GUARD: ret void
 
 ; CHECK_TRACE_PC_GUARD-LABEL: define void @CallViaVptr
 ; CHECK_TRACE_PC_GUARD: call void @__sanitizer_cov_trace_pc_indir
 ; CHECK_TRACE_PC_GUARD: call void @__sanitizer_cov_trace_pc_indir
 ; CHECK_TRACE_PC_GUARD: ret void
 
-; CHECK_TRACE_PC_GUARD-LABEL: define internal void @sancov.module_ctor() {
-
+; CHECK_TRACE_PC_GUARD-LABEL: define internal void @sancov.module_ctor_trace_pc_guard() {