[SanitizerCoverage] Create comdat for global arrays.

[morehouse]

Summary

Place global arrays in comdat sections with their associated functions.
This makes sure they are stripped along with the functions they
reference, even on the BFD linker.

[eugenis]

LGTM
FTR, http://llvm.org/docs/LangRef.html#associated-metadata mentions that comdat is "recommended".
It would be nice to factor this out into a utility function (create associated global for a global) and use it in asan, too.

[eugenis]

Btw what if getUniqueModuleId returns ""? It may happens for files without external symbols.
I think we need to fall back to non-GCed counters.

[morehouse]

• Fallback to counter suffix if getUniqueModuleId fails.

[morehouse]

• Add refactoring TODO.

[morehouse]

• Only set comdats for ELF.
• Fallback to using module ID when getUniqueModuleId fails.

[morehouse]

• Rebase

[morehouse]

• Rebase properly.

[eugenis]

llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

582 ↗

(On Diff #164958)

I don't think this is correct - getModuleIdentifier() is even less unique then getUniqueModuleId, and comdat names for local linkage functions really have to be unique.

Imagine two files with the same relative path, no exported symbols, defining local functions with the same name. We don't want them to share a comdat.

I think this case needs to fallback to non-comdat non-associated instrumentation (the one that suppressed linker GC).

[morehouse]

llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

582 ↗

(On Diff #164958)

Ah, I see. Will revert.

Hopefully suppressing GC for that case won't cause any regressions (e.g., https://github.com/google/sanitizers/issues/971).

[morehouse]

llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

582 ↗

(On Diff #164958)

Hmm... That approach gives me link errors again:

/usr/bin/ld: __sancov_pcs has both ordered [`__sancov_pcs[SSL_COMP_get_name]' in BUILD/libssl.a(ssl_ciph.o)] and unordered [`__sancov_pcs' in BUILD/libssl.a(ssl_ciph.o)] sections

I guess the linker doesn't like having some arrays in comdats and some not in comdats within the same section.

[eugenis]

llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

582 ↗

(On Diff #164958)

Never seen this error before. Does the linker treat names with a dot as ordered by the part of the name that follows the dot? What if you replace the dot with something else?

[morehouse]

llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

582 ↗

(On Diff #164958)

I don't think there's any dots in the comdat name or the section name... In the case above the section name is __sancov_pcs and the comdat name is SSL_COMP_get_name.

[morehouse]

• Do not set comdat for local-linkage functions without a unique module ID

[morehouse]

The error was actually caused by setting associated metadata for some arrays and not others. By always setting the metadata, we can avoid the issue.

@eugenis: PTAL

[eugenis]

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

574

Don't call getUniqueModuleId for every function in a module, it's not exactly O(1).

[morehouse]

• Call getUniqueModuleId once per module.

[eugenis]

LGTM, I'm almost 100% sure this is safe, but I don't remember all the linker quirks around associated sections now.

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

 }
 
 GlobalVariable *SanitizerCoverageModule::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 (auto Comdat = F.getComdat())
+  if (auto Comdat = F.getComdat()) {
     Array->setComdat(Comdat);
+  } else {
+    assert(F.hasName());
+    std::string Name = F.getName();
+    if (F.hasLocalLinkage())
+      Name += getUniqueModuleId(CurModule);
+    Comdat = CurModule->getOrInsertComdat(Name);
+    // Make this IMAGE_COMDAT_SELECT_NODUPLICATES on COFF. Also upgrade private
+    // linkage to internal linkage so that a symbol table entry is emitted. This
+    // is necessary in order to create the comdat group.
+    if (TargetTriple.isOSBinFormatCOFF()) {
+      Comdat->setSelectionKind(Comdat::NoDuplicates);
+      if (F.hasPrivateLinkage())
+        F.setLinkage(GlobalValue::InternalLinkage);
+    }
+    F.setComdat(Comdat);
+    Array->setComdat(Comdat);
+  }
   Array->setSection(getSectionName(Section));
   Array->setAlignment(Ty->isPointerTy() ? DL->getPointerSize()
                                         : Ty->getPrimitiveSizeInBits() / 8);
+  GlobalsToAppendToCompilerUsed.push_back(Array);
+  MDNode *MD = MDNode::get(F.getContext(), ValueAsMetadata::get(&F));
+  Array->addMetadata(LLVMContext::MD_associated, *MD);
   return Array;
 }
 
 GlobalVariable *
 SanitizerCoverageModule::CreatePCArray(Function &F,
                                        ArrayRef<BasicBlock *> AllBlocks) {
   size_t N = AllBlocks.size();
   assert(N);

 }
 
 void SanitizerCoverageModule::CreateFunctionLocalArrays(
     Function &F, ArrayRef<BasicBlock *> AllBlocks) {
   if (Options.TracePCGuard) {
     FunctionGuardArray = CreateFunctionLocalArrayInSection(
         AllBlocks.size(), F, Int32Ty, SanCovGuardsSectionName);
     GlobalsToAppendToUsed.push_back(FunctionGuardArray);
-    GlobalsToAppendToCompilerUsed.push_back(FunctionGuardArray);
-    MDNode *MD = MDNode::get(F.getContext(), ValueAsMetadata::get(&F));
-    FunctionGuardArray->addMetadata(LLVMContext::MD_associated, *MD);
   }
-  if (Options.Inline8bitCounters) {
+  if (Options.Inline8bitCounters)
     Function8bitCounterArray = CreateFunctionLocalArrayInSection(
         AllBlocks.size(), F, Int8Ty, SanCovCountersSectionName);
-    GlobalsToAppendToCompilerUsed.push_back(Function8bitCounterArray);
-    MDNode *MD = MDNode::get(F.getContext(), ValueAsMetadata::get(&F));
-    Function8bitCounterArray->addMetadata(LLVMContext::MD_associated, *MD);
-  }
-  if (Options.PCTable) {
+  if (Options.PCTable)
     FunctionPCsArray = CreatePCArray(F, AllBlocks);
-    GlobalsToAppendToCompilerUsed.push_back(FunctionPCsArray);
-    MDNode *MD = MDNode::get(F.getContext(), ValueAsMetadata::get(&F));
-    FunctionPCsArray->addMetadata(LLVMContext::MD_associated, *MD);
-  }
 }
 
 bool SanitizerCoverageModule::InjectCoverage(Function &F,
                                              ArrayRef<BasicBlock *> AllBlocks,
                                              bool IsLeafFunc) {
   if (AllBlocks.empty()) return false;
   CreateFunctionLocalArrays(F, AllBlocks);
   for (size_t i = 0, N = AllBlocks.size(); i < N; i++)

llvm/test/Instrumentation/SanitizerCoverage/div-tracing.ll

 ; Test -sanitizer-coverage-trace-divs=1
 ; RUN: opt < %s -sancov -sanitizer-coverage-level=1 -sanitizer-coverage-trace-divs=1  -S | FileCheck %s
 
 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 i32 @div_a_b(i32 %a, i32 %b) local_unnamed_addr {
 entry:
   %div = sdiv i32 %a, %b
   ret i32 %div
 }
 
-; CHECK-LABEL: div_a_b
+; CHECK-LABEL: @div_a_b
 ; CHECK: call void @__sanitizer_cov_trace_div4(i32 %b)
 ; CHECK: ret
 
 
 define i32 @div_a_10(i32 %a) local_unnamed_addr {
 entry:
   %div = sdiv i32 %a, 10
   ret i32 %div
 }
 
-; CHECK-LABEL: div_a_10
+; CHECK-LABEL: @div_a_10
 ; CHECK-NOT: __sanitizer_cov_trace_div
 ; CHECK: ret
 
 define i64 @div_a_b_64(i64 %a, i64 %b) local_unnamed_addr {
 entry:
   %div = udiv i64 %a, %b
   ret i64 %div
 }
 
-; CHECK-LABEL: div_a_b
+; CHECK-LABEL: @div_a_b_64
 ; CHECK: call void @__sanitizer_cov_trace_div8(i64 %b)
 ; CHECK: ret
 

llvm/test/Instrumentation/SanitizerCoverage/gep-tracing.ll

 entry:
   %idxprom = sext i32 %j to i64
   %idxprom1 = sext i32 %i to i64
   %arrayidx2 = getelementptr inbounds [1000 x i32], [1000 x i32]* %a, i64 %idxprom1, i64 %idxprom
   store i32 0, i32* %arrayidx2, align 4
   ret void
 }
 
-; CHECK-LABEL: define void @gep_2([1000 x i32]* nocapture %a, i32 %i, i32 %j) {
+; CHECK-LABEL: define void @gep_2([1000 x i32]* nocapture %a, i32 %i, i32 %j)
 ; CHECK: call void @__sanitizer_cov_trace_gep(i64 %idxprom1)
 ; CHECK: call void @__sanitizer_cov_trace_gep(i64 %idxprom)
 ; CHECK: ret void
 
 ; Just make sure we don't insturment this one and don't crash
 define void @gep_3(<2 x i8*> %a, i32 %i, i32 %j) {
 entry:
   %0 = getelementptr i8, <2 x i8*> %a, <2 x i64> <i64 8, i64 8>
   ret void
 }
 

llvm/test/Instrumentation/SanitizerCoverage/inline-8bit-counters.ll

 ; Test -sanitizer-coverage-inline-8bit-counters=1
 ; RUN: opt < %s -sancov -sanitizer-coverage-level=1 -sanitizer-coverage-inline-8bit-counters=1  -S | FileCheck %s
 
 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() {
 entry:
-; CHECK: section "__sancov_cntrs", align 1
+; CHECK: section "__sancov_cntrs", comdat($foo), align 1
 ; CHECK:  %0 = load i8, i8* getelementptr inbounds ([1 x i8], [1 x i8]* @__sancov_gen_, i64 0, i64 0), !nosanitize
 ; CHECK:  %1 = add i8 %0, 1
 ; CHECK:  store i8 %1, i8* getelementptr inbounds ([1 x i8], [1 x i8]* @__sancov_gen_, i64 0, i64 0), !nosanitize
   ret void
 }
 ; CHECK: call void @__sanitizer_cov_8bit_counters_init(i8* bitcast (i8** @__start___sancov_cntrs to i8*), i8* bitcast (i8** @__stop___sancov_cntrs to i8*))

llvm/test/Instrumentation/SanitizerCoverage/pc-table.ll

entry:
   if.then:                                          ; preds = %entry
   store i32 0, i32* %a, align 4
   br label %if.end
 
   if.end:                                           ; preds = %entry, %if.then
   ret void
 }
 
-; CHECK: private constant [6 x i64*] [{{.*}}@foo{{.*}}blockaddress{{.*}}blockaddress{{.*}}], section "__sancov_pcs", align 8
+; CHECK: private constant [6 x i64*] [{{.*}}@foo{{.*}}blockaddress{{.*}}blockaddress{{.*}}], section "__sancov_pcs", comdat($foo), align 8
 ; CHECK: define internal void @sancov.module_ctor
 ; CHECK: call void @__sanitizer_cov
 ; CHECK: call void @__sanitizer_cov_pcs_init

llvm/test/Instrumentation/SanitizerCoverage/tracing.ll

 ; CHECK_PC-LABEL: define void @foo
 ; CHECK_PC: call void @__sanitizer_cov_trace_pc
 ; CHECK_PC: call void @__sanitizer_cov_trace_pc
 ; CHECK_PC: call void @__sanitizer_cov_trace_pc
 ; CHECK_PC-NOT: call void @__sanitizer_cov_trace_pc
 ; CHECK_PC: ret void
 ; CHECK_PC-NOT: call void @__sanitizer_cov_module_init
 
-; CHECK_PC_GUARD: section "__sancov_guards", align 4
+; CHECK_PC_GUARD: section "__sancov_guards", comdat($foo), align 4
 ; CHECK_PC_GUARD-LABEL: define void @foo
 ; CHECK_PC_GUARD: call void @__sanitizer_cov_trace_pc_guard
 ; CHECK_PC_GUARD: call void @__sanitizer_cov_trace_pc_guard
 ; CHECK_PC_GUARD: call void @__sanitizer_cov_trace_pc_guard
 ; CHECK_PC_GUARD-NOT: call void @__sanitizer_cov_trace_pc
 ; CHECK_PC_GUARD: ret void
 ; CHECK_PC_GUARD-LABEL: @external_bar
 ; CHECK_PC_GUARD-NOT: call void @__sanitizer_cov_trace_pc