[InstrProfiling] Place __llvm_prf_vnodes and __llvm_prf_names in llvm.used on ELF

[MaskRay]

Summary

__llvm_prf_vnodes and __llvm_prf_names are used by runtime but not
referenced via relocation in the translation unit.

With -z start-stop-gc (LLD 13 (D96914); GNU ld 2.37 https://sourceware.org/bugzilla/show_bug.cgi?id=27451),
the linker does not let __start_/__stop_ references retain their sections.

Place __llvm_prf_vnodes and __llvm_prf_names in llvm.used to make
them retained by the linker.

This patch changes most existing UsedVars cases to CompilerUsedVars
to reflect the ideal state - if the binary format properly supports
section based GC (dead stripping), llvm.compiler.used should be sufficient.

__llvm_prf_vnodes and __llvm_prf_names are switched to UsedVars
since we want them to be unconditionally retained by both compiler and linker.

Behaviors on COFF/Mach-O are not affected.

[MaskRay]

rebase

[davidxl]

compiler-rt/test/profile/Linux/instrprof-value-merge.c

11

Can you add tests for other linkers with similar options?

[MaskRay]

Test GNU ld and gold

[davidxl]

compiler-rt/test/profile/Linux/instrprof-value-merge.c

2

Is it better to split the test into multiple RUN file based tests with only one requires linux and lld? Otherwise it seems that adding this will disable the tests on some bots (freebsd or no lld).

[MaskRay]

Add instrprof-value-merge-lld.c with REQUIRES: lld-available

[davidxl]

LGTM (the a.out file is probably accidentally added).

[MaskRay]

Drop accidental a.out

[thakis]

This breaks instrprof-value-merge.c on our bots (linux only): https://ci.chromium.org/ui/p/chromium/builders/try/linux_upload_clang/1394/overview

It fails in stage 0, which is a pretty vanilla build.

I reverted this for now in 64f5d7e9725b1e1b9d70628e741aa4390795d510

Please let me know if you need help reproducing :)

[MaskRay]

In D97649#2600311, @thakis wrote:

This breaks instrprof-value-merge.c on our bots (linux only): https://ci.chromium.org/ui/p/chromium/builders/try/linux_upload_clang/1394/overview

It fails in stage 0, which is a pretty vanilla build.

I reverted this for now in 64f5d7e9725b1e1b9d70628e741aa4390795d510

Please let me know if you need help reproducing :)

How can I reproduce it?

This works in normal builds.

[MaskRay]

In D97649#2600694, @MaskRay wrote:

In D97649#2600311, @thakis wrote:

This breaks instrprof-value-merge.c on our bots (linux only): https://ci.chromium.org/ui/p/chromium/builders/try/linux_upload_clang/1394/overview

It fails in stage 0, which is a pretty vanilla build.

I reverted this for now in 64f5d7e9725b1e1b9d70628e741aa4390795d510

Please let me know if you need help reproducing :)

How can I reproduce it?

This works in normal builds.

@thakis said this is a Ubuntu 14.04 bot. I guess it may have an old binutils where --gc-sections might never work for value profiling properly.
(It reminds me of https://reviews.llvm.org/D97110#2583184 a binutils ~2.28 bug)

I'll omit --gc-sections in instrprof-value-merge.c for now.

[MaskRay]

Precommit improved tests

compiler-rt/test/profile/Linux/instrprof-value-merge.c

+// REQUIRES: linux, lld-available
 // RUN: %clang_pgogen -o %t -O3 %s
 // RUN: rm -rf %t.profdir
 // RUN: env LLVM_PROFILE_FILE=%t.profdir/default_%m.profraw %run %t
 // RUN: env LLVM_PROFILE_FILE=%t.profdir/default_%m.profraw %run %t
 // RUN: env LLVM_PROFILE_FILE=%t.profdir/default_%m.profraw %run %t
 // RUN: env LLVM_PROFILE_FILE=%t.profdir/default_%m.profraw %run %t 1
 // RUN: env LLVM_PROFILE_FILE=%t.profdir/default_%m.profraw %run %t 1
 // RUN: llvm-profdata show -counts -function=main -ic-targets -memop-sizes %t.profdir/default_*.profraw | FileCheck %s
 
+// RUN: %clang_pgogen -o %t -O3 %s -fuse-ld=lld -ffunction-sections -fdata-sections -Wl,--gc-sections -z start-stop-gc
+// RUN: rm -rf %t.profdir
+// RUN: env LLVM_PROFILE_FILE=%t.profdir/default_%m.profraw %run %t
+// RUN: env LLVM_PROFILE_FILE=%t.profdir/default_%m.profraw %run %t
+// RUN: env LLVM_PROFILE_FILE=%t.profdir/default_%m.profraw %run %t
+// RUN: env LLVM_PROFILE_FILE=%t.profdir/default_%m.profraw %run %t 1
+// RUN: env LLVM_PROFILE_FILE=%t.profdir/default_%m.profraw %run %t 1
+// RUN: llvm-profdata show -counts -function=main -ic-targets -memop-sizes %t.profdir/default_*.profraw | FileCheck %s
+
 #include <string.h>
 
 void (*f0)();
 void (*f1)();
 void (*f2)();
 
 char dst[200];
 char src[200];

llvm/include/llvm/Transforms/Instrumentation/InstrProfiling.h

struct PerFunctionProfileData {
     GlobalVariable *RegionCounters = nullptr;
     GlobalVariable *DataVar = nullptr;
 
     PerFunctionProfileData() {
       memset(NumValueSites, 0, sizeof(uint32_t) * (IPVK_Last + 1));
     }
   };
   DenseMap<GlobalVariable *, PerFunctionProfileData> ProfileDataMap;
+  std::vector<GlobalValue *> CompilerUsedVars;
   std::vector<GlobalValue *> UsedVars;
   std::vector<GlobalVariable *> ReferencedNames;
   GlobalVariable *NamesVar;
   size_t NamesSize;
 
   // Is this lowering for the context-sensitive instrumentation.
   bool IsCS;
 

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

 
 bool InstrProfiling::run(
     Module &M, std::function<const TargetLibraryInfo &(Function &F)> GetTLI) {
   this->M = &M;
   this->GetTLI = std::move(GetTLI);
   NamesVar = nullptr;
   NamesSize = 0;
   ProfileDataMap.clear();
+  CompilerUsedVars.clear();
   UsedVars.clear();
   TT = Triple(M.getTargetTriple());
 
   // Emit the runtime hook even if no counters are present.
   bool MadeChange = emitRuntimeHook();
 
   // Improve compile time by avoiding linear scans when there is no work.
   GlobalVariable *CoverageNamesVar =
#include "llvm/ProfileData/InstrProfData.inc"
   MaybeSetComdat(Data);
   Data->setLinkage(Linkage);
 
   PD.RegionCounters = CounterPtr;
   PD.DataVar = Data;
   ProfileDataMap[NamePtr] = PD;
 
   // Mark the data variable as used so that it isn't stripped out.
-  UsedVars.push_back(Data);
+  CompilerUsedVars.push_back(Data);
   // Now that the linkage set by the FE has been passed to the data and counter
   // variables, reset Name variable's linkage and visibility to private so that
   // it can be removed later by the compiler.
   NamePtr->setLinkage(GlobalValue::PrivateLinkage);
   // Collect the referenced names to be used by emitNameData.
   ReferencedNames.push_back(NamePtr);
 
   return CounterPtr;
#include "llvm/ProfileData/InstrProfData.inc"
   auto *VNodeTy = StructType::get(Ctx, makeArrayRef(VNodeTypes));
 
   ArrayType *VNodesTy = ArrayType::get(VNodeTy, NumCounters);
   auto *VNodesVar = new GlobalVariable(
       *M, VNodesTy, false, GlobalValue::PrivateLinkage,
       Constant::getNullValue(VNodesTy), getInstrProfVNodesVarName());
   VNodesVar->setSection(
       getInstrProfSectionName(IPSK_vnodes, TT.getObjectFormat()));
+  // VNodesVar is used by runtime but not referenced via relocation by other
+  // sections. Conservatively make it linker retained.
   UsedVars.push_back(VNodesVar);
 }
 
 void InstrProfiling::emitNameData() {
   std::string UncompressedData;
 
   if (ReferencedNames.empty())
     return;
NamesVar = new GlobalVariable(*M, NamesVal->getType(), true,
                                 getInstrProfNamesVarName());
   NamesSize = CompressedNameStr.size();
   NamesVar->setSection(
       getInstrProfSectionName(IPSK_name, TT.getObjectFormat()));
   // On COFF, it's important to reduce the alignment down to 1 to prevent the
   // linker from inserting padding before the start of the names section or
   // between names entries.
   NamesVar->setAlignment(Align(1));
+  // NamesVar is used by runtime but not referenced via relocation by other
+  // sections. Conservatively make it linker retained.
   UsedVars.push_back(NamesVar);
 
   for (auto *NamePtr : ReferencedNames)
     NamePtr->eraseFromParent();
 }
 
 void InstrProfiling::emitRegistration() {
   if (!needsRuntimeRegistrationOfSectionRange(TT))
if (Options.NoRedZone)
     RegisterF->addFnAttr(Attribute::NoRedZone);
 
   auto *RuntimeRegisterTy = FunctionType::get(VoidTy, VoidPtrTy, false);
   auto *RuntimeRegisterF =
       Function::Create(RuntimeRegisterTy, GlobalVariable::ExternalLinkage,
                        getInstrProfRegFuncName(), M);
 
   IRBuilder<> IRB(BasicBlock::Create(M->getContext(), "", RegisterF));
+  for (Value *Data : CompilerUsedVars)
+    if (Data != NamesVar && !isa<Function>(Data))
+      IRB.CreateCall(RuntimeRegisterF, IRB.CreateBitCast(Data, VoidPtrTy));
   for (Value *Data : UsedVars)
     if (Data != NamesVar && !isa<Function>(Data))
       IRB.CreateCall(RuntimeRegisterF, IRB.CreateBitCast(Data, VoidPtrTy));
 
   if (NamesVar) {
     Type *ParamTypes[] = {VoidPtrTy, Int64Ty};
     auto *NamesRegisterTy =
         FunctionType::get(VoidTy, makeArrayRef(ParamTypes), false);
bool InstrProfiling::emitRuntimeHook() {
   if (TT.supportsCOMDAT())
     User->setComdat(M->getOrInsertComdat(User->getName()));
 
   IRBuilder<> IRB(BasicBlock::Create(M->getContext(), "", User));
   auto *Load = IRB.CreateLoad(Int32Ty, Var);
   IRB.CreateRet(Load);
 
   // Mark the user variable as used so that it isn't stripped out.
-  UsedVars.push_back(User);
+  CompilerUsedVars.push_back(User);
   return true;
 }
 
 void InstrProfiling::emitUses() {
   // The metadata sections are parallel arrays. Optimizers (e.g.
   // GlobalOpt/ConstantMerge) may not discard associated sections as a unit, so
   // we conservatively retain all unconditionally in the compiler.
   //
   // On ELF, the linker can guarantee the associated sections will be retained
   // or discarded as a unit, so llvm.compiler.used is sufficient. Otherwise,
   // conservatively make all of them retained by the linker.
   if (TT.isOSBinFormatELF())
-    appendToCompilerUsed(*M, UsedVars);
+    appendToCompilerUsed(*M, CompilerUsedVars);
   else
-    appendToUsed(*M, UsedVars);
+    appendToUsed(*M, CompilerUsedVars);
+
+  // We do not add proper references from used metadata sections to NamesVar and
+  // VNodesVar, so we have to be conservative and place them in llvm.used
+  // regardless of the target,
 }
 
 void InstrProfiling::emitInitialization() {
   // Create ProfileFileName variable. Don't don't this for the
   // context-sensitive instrumentation lowering: This lowering is after
   // LTO/ThinLTO linking. Pass PGOInstrumentationGenCreateVar should
   // have already create the variable before LTO/ThinLTO linking.
   if (!IsCS)

llvm/test/Instrumentation/InstrProfiling/icall.ll

 
 ; STATIC: @__profvp_foo = private global [1 x i64] zeroinitializer, section "{{[^"]+}}"
 ; STATIC: @__profvp_bar = private global [1 x i64] zeroinitializer, section "{{[^"]+}}", comdat($__profd_bar)
 ; STATIC: @__llvm_prf_vnodes
 
 ; DYN-NOT: @__profvp_foo
 ; DYN-NOT: @__llvm_prf_vnodes
 
+;; __llvm_prf_vnodes and __llvm_prf_nm are not referenced by other metadata sections.
+;; We have to conservatively place them in llvm.used.
+; STATIC:      @llvm.used = appending global
+; STATIC-SAME:   @__llvm_prf_vnodes
+; STATIC-SAME:   @__llvm_prf_nm
+
 ; STATIC: call void @__llvm_profile_instrument_target(i64 %3, i8* bitcast ({ i64, i64, i64*, i8*, i8*, i32, [2 x i16] }* @__profd_foo to i8*), i32 0)
 ; STATIC-EXT: call void @__llvm_profile_instrument_target(i64 %3, i8* bitcast ({ i64, i64, i64*, i8*, i8*, i32, [2 x i16] }* @__profd_foo to i8*), i32 zeroext 0)
 ; STATIC-SEXT: call void @__llvm_profile_instrument_target(i64 %3, i8* bitcast ({ i64, i64, i64*, i8*, i8*, i32, [2 x i16] }* @__profd_foo to i8*), i32 signext 0)
 
 ; STATIC: declare void @__llvm_profile_instrument_target(i64, i8*, i32)
 ; STATIC-EXT: declare void @__llvm_profile_instrument_target(i64, i8*, i32 zeroext)
 ; STATIC-SEXT: declare void @__llvm_profile_instrument_target(i64, i8*, i32 signext)