From 6cd8e520a98805c2bdd033c5ba71985ddc7dde16 Mon Sep 17 00:00:00 2001 From: Fangrui Song Date: Thu, 25 Feb 2021 11:59:23 -0800 Subject: [PATCH] [SanitizerCoverage] Drop !associated on metadata sections 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). Reviewed By: morehouse, rnk, vitalybuka Differential Revision: https://reviews.llvm.org/D97430 --- include/llvm/Transforms/Instrumentation.h | 3 +-- lib/Transforms/IPO/SampleProfileProbe.cpp | 5 ++-- .../Instrumentation/Instrumentation.cpp | 22 ++++----------- .../Instrumentation/SanitizerCoverage.cpp | 8 +++--- .../SanitizerCoverage/inline-bool-flag.ll | 5 ++-- ...bol-nocomdat.ll => interposable-symbol.ll} | 27 +++++++++++-------- .../SanitizerCoverage/trace-pc-guard.ll | 12 ++++----- 7 files changed, 35 insertions(+), 47 deletions(-) rename test/Instrumentation/SanitizerCoverage/{interposable-symbol-nocomdat.ll => interposable-symbol.ll} (64%) diff --git a/include/llvm/Transforms/Instrumentation.h b/include/llvm/Transforms/Instrumentation.h index c960d5b0ab5..03108bacb0d 100644 --- a/include/llvm/Transforms/Instrumentation.h +++ b/include/llvm/Transforms/Instrumentation.h @@ -46,8 +46,7 @@ GlobalVariable *createPrivateGlobalForString(Module &M, StringRef Str, // 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 { diff --git a/lib/Transforms/IPO/SampleProfileProbe.cpp b/lib/Transforms/IPO/SampleProfileProbe.cpp index a885c3ee4de..d878d44bf8e 100644 --- a/lib/Transforms/IPO/SampleProfileProbe.cpp +++ b/lib/Transforms/IPO/SampleProfileProbe.cpp @@ -364,9 +364,8 @@ void SampleProfileProber::instrumentOneFunc(Function &F, TargetMachine *TM) { 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); } } } diff --git a/lib/Transforms/Instrumentation/Instrumentation.cpp b/lib/Transforms/Instrumentation/Instrumentation.cpp index cfdf3cad97f..1fe1e3d1abe 100644 --- a/lib/Transforms/Instrumentation/Instrumentation.cpp +++ b/lib/Transforms/Instrumentation/Instrumentation.cpp @@ -73,28 +73,16 @@ GlobalVariable *llvm::createPrivateGlobalForString(Module &M, StringRef Str, 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; diff --git a/lib/Transforms/Instrumentation/SanitizerCoverage.cpp b/lib/Transforms/Instrumentation/SanitizerCoverage.cpp index b4b34450ecf..69512fd9294 100644 --- a/lib/Transforms/Instrumentation/SanitizerCoverage.cpp +++ b/lib/Transforms/Instrumentation/SanitizerCoverage.cpp @@ -676,16 +676,14 @@ GlobalVariable *ModuleSanitizerCoverage::CreateFunctionLocalArrayInSection( *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; } diff --git a/test/Instrumentation/SanitizerCoverage/inline-bool-flag.ll b/test/Instrumentation/SanitizerCoverage/inline-bool-flag.ll index 87739db4b07..e711d96a5a4 100644 --- a/test/Instrumentation/SanitizerCoverage/inline-bool-flag.ll +++ b/test/Instrumentation/SanitizerCoverage/inline-bool-flag.ll @@ -2,8 +2,8 @@ ; 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: $foo = comdat any -; 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{{$}} ; CHECK-NOT: @llvm.used = ; CHECK: @llvm.compiler.used = appending global [1 x i8*] [i8* bitcast ([1 x i1]* @__sancov_gen_ to i8*)], section "llvm.metadata" @@ -26,5 +26,4 @@ entry: } ; CHECK: call void @__sanitizer_cov_bool_flag_init(i1* @__start___sancov_bools, i1* @__stop___sancov_bools) -; CHECK: !0 = !{void ()* @foo} ; CHECK: ![[#EMPTY]] = !{} diff --git a/test/Instrumentation/SanitizerCoverage/interposable-symbol-nocomdat.ll b/test/Instrumentation/SanitizerCoverage/interposable-symbol.ll similarity index 64% rename from test/Instrumentation/SanitizerCoverage/interposable-symbol-nocomdat.ll rename to test/Instrumentation/SanitizerCoverage/interposable-symbol.ll index 6740b45c5ce..ba528873d28 100644 --- a/test/Instrumentation/SanitizerCoverage/interposable-symbol-nocomdat.ll +++ b/test/Instrumentation/SanitizerCoverage/interposable-symbol.ll @@ -1,8 +1,8 @@ ; 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: @@ -31,15 +31,20 @@ 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 { diff --git a/test/Instrumentation/SanitizerCoverage/trace-pc-guard.ll b/test/Instrumentation/SanitizerCoverage/trace-pc-guard.ll index ffaef122c13..a3b3aa834c7 100644 --- a/test/Instrumentation/SanitizerCoverage/trace-pc-guard.ll +++ b/test/Instrumentation/SanitizerCoverage/trace-pc-guard.ll @@ -4,13 +4,13 @@ ; RUN: opt < %s -sancov -sanitizer-coverage-level=4 -sanitizer-coverage-trace-pc-guard -mtriple=aarch64-apple-darwin -S -enable-new-pm=0 | FileCheck %s --check-prefixes=CHECK,MACHO ; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=4 -sanitizer-coverage-trace-pc-guard -mtriple=aarch64-apple-darwin -S | FileCheck %s --check-prefixes=CHECK,MACHO -; ELF: $foo = comdat any -; ELF: $CallViaVptr = comdat any -; ELF: @__sancov_gen_ = private global [3 x i32] zeroinitializer, section "__sancov_guards", comdat($foo), align 4, !associated !0 -; ELF-NEXT: @__sancov_gen_.1 = private global [1 x i32] zeroinitializer, section "__sancov_guards", comdat($CallViaVptr), align 4, !associated !1 +; ELF: $foo = comdat noduplicates +; ELF: $CallViaVptr = comdat noduplicates +; ELF: @__sancov_gen_ = private global [3 x i32] zeroinitializer, section "__sancov_guards", comdat($foo), align 4{{$}} +; ELF-NEXT: @__sancov_gen_.1 = private global [1 x i32] zeroinitializer, section "__sancov_guards", comdat($CallViaVptr), align 4{{$}} -; MACHO: @__sancov_gen_ = private global [3 x i32] zeroinitializer, section "__DATA,__sancov_guards", align 4, !associated !0 -; MACHO-NEXT: @__sancov_gen_.1 = private global [1 x i32] zeroinitializer, section "__DATA,__sancov_guards", align 4, !associated !1 +; MACHO: @__sancov_gen_ = private global [3 x i32] zeroinitializer, section "__DATA,__sancov_guards", align 4{{$}} +; MACHO-NEXT: @__sancov_gen_.1 = private global [1 x i32] zeroinitializer, section "__DATA,__sancov_guards", align 4{{$}} ; ELF-NOT: @llvm.used = ; MACHO: @llvm.used = appending global [2 x i8*] [i8* bitcast ([3 x i32]* @__sancov_gen_ to i8*), i8* bitcast ([1 x i32]* @__sancov_gen_.1 to i8*)], section "llvm.metadata"