From 9ab9a9595b1bbcf360de619f7c9c17f4340caa52 Mon Sep 17 00:00:00 2001 From: Fangrui Song Date: Tue, 24 Aug 2021 20:14:02 -0700 Subject: [PATCH] [InstrProfiling] Keep profd non-private for non-renamable comdat functions The NS==0 condition used by D103717 missed a corner case: if the current copy does not have a hash suffix (e.g. weak_odr), a copy with value profiling (with a different CFG) may exist. This is super rare, but is possible with pre-inlining PGO instrumentation (which can make a weak_odr function inlines its callees differently, sometimes with value profiling while sometimes without). If the current copy with private profd is prevailing, the non-prevailing copy may get an undefined symbol if a caller inlining the non-prevailing function references its profd. If the other copy with non-private profd is prevailing, the current copy may cause a "relocation to discarded section" linker error. The fix is straightforward: just keep non-private profd in such a `DataReferencedByCode` case. With this change, a stage 2 (`-DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_BUILD_INSTRUMENTED=IR`) clang is 0.08% larger (172431496/172286720-1). `stat -c %s **/*.o | awk '{s+=$1}END{print s}' is 0.026% larger. The majority of D103717's benefits remains. Reviewed By: xur Differential Revision: https://reviews.llvm.org/D108432 --- .../Transforms/Instrumentation/InstrProfiling.cpp | 26 ++++++++++++++++------ llvm/test/Transforms/PGOProfile/comdat.ll | 7 ++++-- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp index d2620fd..a335824 100644 --- a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp +++ b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp @@ -758,14 +758,18 @@ void InstrProfiling::lowerCoverageData(GlobalVariable *CoverageNamesVar) { } /// Get the name of a profiling variable for a particular function. -static std::string getVarName(InstrProfIncrementInst *Inc, StringRef Prefix) { +static std::string getVarName(InstrProfIncrementInst *Inc, StringRef Prefix, + bool &Renamed) { StringRef NamePrefix = getInstrProfNameVarPrefix(); StringRef Name = Inc->getName()->getName().substr(NamePrefix.size()); Function *F = Inc->getParent()->getParent(); Module *M = F->getParent(); if (!DoHashBasedCounterSplit || !isIRPGOFlagSet(M) || - !canRenameComdatFunc(*F)) + !canRenameComdatFunc(*F)) { + Renamed = false; return (Prefix + Name).str(); + } + Renamed = true; uint64_t FuncHash = Inc->getHash()->getZExtValue(); SmallVector HashPostfix; if (Name.endswith((Twine(".") + Twine(FuncHash)).toStringRef(HashPostfix))) @@ -878,8 +882,11 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) { // discarded. bool DataReferencedByCode = profDataReferencedByCode(*M); bool NeedComdat = needsComdatForCounter(*Fn, *M); - std::string CntsVarName = getVarName(Inc, getInstrProfCountersVarPrefix()); - std::string DataVarName = getVarName(Inc, getInstrProfDataVarPrefix()); + bool Renamed; + std::string CntsVarName = + getVarName(Inc, getInstrProfCountersVarPrefix(), Renamed); + std::string DataVarName = + getVarName(Inc, getInstrProfDataVarPrefix(), Renamed); auto MaybeSetComdat = [&](GlobalVariable *GV) { bool UseComdat = (NeedComdat || TT.isOSBinFormatELF()); if (UseComdat) { @@ -920,7 +927,7 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) { ArrayType *ValuesTy = ArrayType::get(Type::getInt64Ty(Ctx), NS); auto *ValuesVar = new GlobalVariable( *M, ValuesTy, false, Linkage, Constant::getNullValue(ValuesTy), - getVarName(Inc, getInstrProfValuesVarPrefix())); + getVarName(Inc, getInstrProfValuesVarPrefix(), Renamed)); ValuesVar->setVisibility(Visibility); ValuesVar->setSection( getInstrProfSectionName(IPSK_vals, TT.getObjectFormat())); @@ -955,8 +962,13 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) { // // On COFF, a comdat leader cannot be local so we require DataReferencedByCode // to be false. - if (NS == 0 && (TT.isOSBinFormatELF() || - (!DataReferencedByCode && TT.isOSBinFormatCOFF()))) { + // + // If profd is in a deduplicate comdat, NS==0 with a hash suffix guarantees + // that other copies must have the same CFG and cannot have value profiling. + // If no hash suffix, other profd copies may be referenced by code. + if (NS == 0 && !(DataReferencedByCode && NeedComdat && !Renamed) && + (TT.isOSBinFormatELF() || + (!DataReferencedByCode && TT.isOSBinFormatCOFF()))) { Linkage = GlobalValue::PrivateLinkage; Visibility = GlobalValue::DefaultVisibility; } diff --git a/llvm/test/Transforms/PGOProfile/comdat.ll b/llvm/test/Transforms/PGOProfile/comdat.ll index 6a97203..9f5c0ee 100644 --- a/llvm/test/Transforms/PGOProfile/comdat.ll +++ b/llvm/test/Transforms/PGOProfile/comdat.ll @@ -16,9 +16,12 @@ define linkonce_odr void @linkonceodr() comdat { ret void } -;; weakodr in a comdat is not renamed. +;; weakodr in a comdat is not renamed. There is no guarantee that definitions in +;; other modules don't have value profiling. profd should be conservatively +;; non-private to prevent a caller from referencing a non-prevailing profd, +;; causing a linker error. ; ELF: @__profc_weakodr = weak_odr hidden global {{.*}} comdat, align 8 -; ELF: @__profd_weakodr = private global {{.*}} comdat($__profc_weakodr), align 8 +; ELF: @__profd_weakodr = weak_odr hidden global {{.*}} comdat($__profc_weakodr), align 8 ; COFF: @__profc_weakodr = weak_odr hidden global {{.*}} comdat, align 8 ; COFF: @__profd_weakodr = weak_odr hidden global {{.*}} comdat, align 8 define weak_odr void @weakodr() comdat { -- 2.7.4