[sancov] Put .SCOV* sections into the right comdat groups on COFF

[rnk]

Summary

Avoids linker errors about relocations against discarded sections.

This was uncovered during the clang roll here:
https://chromium-review.googlesource.com/c/chromium/src/+/1321863#message-717516acfcf829176f6a2f50980f7a4bdd66469a

After this change, libGLESv2 links successfully for me.

[morehouse]

llvm/lib/Transforms/Instrumentation/Instrumentation.cpp

82 ↗

(On Diff #173037)

Does COFF not have comdat naming collisions for local symbols like ELF does?

[rnk]

llvm/lib/Transforms/Instrumentation/Instrumentation.cpp

82 ↗

(On Diff #173037)

The model is pretty different. On COFF, the comdat group has to have a "leader" symbol. All non-leader symbols in the group are included in the final link only if the leader from their object file prevails.

Actually, adding this unique suffix breaks the LLVM IR verifier, since there is a verifier rule that says that the name of a comdat must match some symbol and that symbol must be in the comdat group. Perhaps the comment should explain that.

[rnk]

• Elaborate on comdats in a comment

[morehouse]

LGTM

llvm/trunk/include/llvm/Transforms/Instrumentation.h

 #include <cassert>
 #include <cstdint>
 #include <limits>
 #include <string>
 #include <vector>
 
 namespace llvm {
 
+class Triple;
 class FunctionPass;
 class ModulePass;
 class OptimizationRemarkEmitter;
 class Comdat;
 
 /// Instrumentation passes often insert conditional checks into entry blocks.
 /// Call this function before splitting the entry block to move instructions
 /// that must remain in the entry block up before the split point. Static
 /// allocas and llvm.localescape calls, for example, must remain in the entry
 /// block.
 BasicBlock::iterator PrepareToSplitEntryBlock(BasicBlock &BB,
                                               BasicBlock::iterator IP);
 
 // 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, const std::string &ModuleId);
+Comdat *GetOrCreateFunctionComdat(Function &F, Triple &T,
+                                  const std::string &ModuleId);
 
 // Insert GCOV profiling instrumentation
 struct GCOVOptions {
   static GCOVOptions getDefault();
 
   // Specify whether to emit .gcno files.
   bool EmitNotes;
 

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

void HWAddressSanitizer::createFrameGlobal(Function &F,
   auto GV = new GlobalVariable(
       M, PtrPairTy, /*isConstantGlobal*/ true, GlobalVariable::PrivateLinkage,
       ConstantStruct::get(PtrPairTy, (Constant *)&F, (Constant *)DescrGV),
       "__hwasan");
   GV->setSection(getFrameSection());
   appendToCompilerUsed(M, GV);
   // Put GV into the F's Comadat so that if F is deleted GV can be deleted too.
   if (&F != HwasanCtorFunction)
-    if (auto Comdat = GetOrCreateFunctionComdat(F, CurModuleUniqueId))
+    if (auto Comdat =
+            GetOrCreateFunctionComdat(F, TargetTriple, CurModuleUniqueId))
       GV->setComdat(Comdat);
 }
 
 Value *HWAddressSanitizer::emitPrologue(IRBuilder<> &IRB,
                                         bool WithFrameRecord) {
   if (!Mapping.InTls)
     return getDynamicShadowNonTls(IRB);
 

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

 //===-- Instrumentation.cpp - TransformUtils Infrastructure ---------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines the common initialization infrastructure for the
 // Instrumentation library.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/Instrumentation.h"
 #include "llvm-c/Initialization.h"
+#include "llvm/ADT/Triple.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Module.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/PassRegistry.h"
 
 using namespace llvm;
 
 /// Moves I before IP. Returns new insert point.
GlobalVariable *GV =
       new GlobalVariable(M, StrConst->getType(), true,
                          GlobalValue::PrivateLinkage, StrConst, NamePrefix);
   if (AllowMerging)
     GV->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
   GV->setAlignment(1);  // Strings may not be merged w/o setting align 1.
   return GV;
 }
 
-Comdat *llvm::GetOrCreateFunctionComdat(Function &F,
+Comdat *llvm::GetOrCreateFunctionComdat(Function &F, Triple &T,
                                         const std::string &ModuleId) {
   if (auto Comdat = F.getComdat()) return Comdat;
   assert(F.hasName());
   Module *M = F.getParent();
   std::string Name = F.getName();
-  if (F.hasLocalLinkage()) {
+
+  // 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;
   }
-  F.setComdat(M->getOrInsertComdat(Name));
-  return F.getComdat();
+
+  // 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())
+    C->setSelectionKind(Comdat::NoDuplicates);
+  F.setComdat(C);
+  return C;
 }
 
 /// initializeInstrumentation - Initialize all passes in the TransformUtils
 /// library.
 void llvm::initializeInstrumentation(PassRegistry &Registry) {
   initializeAddressSanitizerPass(Registry);
   initializeAddressSanitizerModulePass(Registry);
   initializeBoundsCheckingLegacyPassPass(Registry);

llvm/trunk/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 (TargetTriple.isOSBinFormatELF())
-    if (auto Comdat = GetOrCreateFunctionComdat(F, CurModuleUniqueId))
+  if (TargetTriple.supportsCOMDAT())
+    if (auto Comdat =
+            GetOrCreateFunctionComdat(F, TargetTriple, CurModuleUniqueId))
       Array->setComdat(Comdat);
   Array->setSection(getSectionName(Section));
   Array->setAlignment(Ty->isPointerTy() ? DL->getPointerSize()
                                         : Ty->getPrimitiveSizeInBits() / 8);
   GlobalsToAppendToUsed.push_back(Array);
   GlobalsToAppendToCompilerUsed.push_back(Array);
   MDNode *MD = MDNode::get(F.getContext(), ValueAsMetadata::get(&F));
   Array->addMetadata(LLVMContext::MD_associated, *MD);

llvm/trunk/test/Instrumentation/SanitizerCoverage/coff-comdat.ll

+; RUN: opt < %s -sancov -sanitizer-coverage-level=1 -sanitizer-coverage-inline-8bit-counters=1 -sanitizer-coverage-pc-table=1 -S | FileCheck %s
+
+; Make sure we use the right comdat groups for COFF to avoid relocations
+; against discarded sections. Internal linkage functions are also different from
+; ELF. We don't add a module unique identifier.
+
+; Test based on this source:
+; int baz(int);
+; static int __attribute__((noinline)) bar(int x) {
+;   if (x)
+;     return baz(x);
+;   return 0;
+; }
+; int foo(int x) {
+;   if (baz(0))
+;     x = bar(x);
+;   return x;
+; }
+
+; Both new comdats should no duplicates on COFF.
+
+; CHECK: $foo = comdat noduplicates
+; CHECK: $bar = comdat noduplicates
+
+; Tables for 'foo' should be in the 'foo' comdat.
+
+; CHECK: @__sancov_gen_{{.*}} = private global [1 x i8] zeroinitializer, section ".SCOV$CM", comdat($foo), align 1
+
+; CHECK: @__sancov_gen_{{.*}} = private constant [2 x i64*]
+; CHECK-SAME: [i64* bitcast (i32 (i32)* @foo to i64*), i64* inttoptr (i64 1 to i64*)],
+; CHECK-SAME: section ".SCOVP$M", comdat($foo), align 8
+
+; Tables for 'bar' should be in the 'bar' comdat.
+
+; CHECK: @__sancov_gen_{{.*}} = private global [1 x i8] zeroinitializer, section ".SCOV$CM", comdat($bar), align 1
+
+; CHECK: @__sancov_gen_{{.*}} = private constant [2 x i64*]
+; CHECK-SAME: [i64* bitcast (i32 (i32)* @bar to i64*), i64* inttoptr (i64 1 to i64*)],
+; CHECK-SAME: section ".SCOVP$M", comdat($bar), align 8
+
+; 'foo' and 'bar' should be in their new comdat groups.
+
+; CHECK: define dso_local i32 @foo(i32 %x){{.*}} comdat {
+; CHECK: define internal fastcc i32 @bar(i32 %x){{.*}} comdat {
+
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc19.14.26433"
+
+; Function Attrs: nounwind uwtable
+define dso_local i32 @foo(i32 %x) local_unnamed_addr #0 {
+entry:
+  %call = tail call i32 @baz(i32 0) #3
+  %tobool = icmp eq i32 %call, 0
+  br i1 %tobool, label %if.end, label %if.then
+
+if.then:                                          ; preds = %entry
+  %call1 = tail call fastcc i32 @bar(i32 %x)
+  br label %if.end
+
+if.end:                                           ; preds = %entry, %if.then
+  %x.addr.0 = phi i32 [ %call1, %if.then ], [ %x, %entry ]
+  ret i32 %x.addr.0
+}
+
+declare dso_local i32 @baz(i32) local_unnamed_addr #1
+
+; Function Attrs: noinline nounwind uwtable
+define internal fastcc i32 @bar(i32 %x) unnamed_addr #2 {
+entry:
+  %tobool = icmp eq i32 %x, 0
+  br i1 %tobool, label %return, label %if.then
+
+if.then:                                          ; preds = %entry
+  %call = tail call i32 @baz(i32 %x) #3
+  br label %return
+
+return:                                           ; preds = %entry, %if.then
+  %retval.0 = phi i32 [ %call, %if.then ], [ 0, %entry ]
+  ret i32 %retval.0
+}
+
+attributes #0 = { nounwind uwtable }
+attributes #1 = { "asdf" }
+attributes #2 = { noinline nounwind uwtable }
+attributes #3 = { nounwind }