[PATCH] D103717: [InstrProfiling][ELF] Make __profd_ private if the function does not use value profiling

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:891
+  uint64_t NS = 0;
+  for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
+    NS += PD.NumValueSites[Kind];
----------------
davidxl wrote:
> should this code by guarded by whether value profile is enabled?
For performance? 

The code below iterates over the array unconditionally
```
  for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
    Int16ArrayVals[Kind] = ConstantInt::get(Int16Ty, PD.NumValueSites[Kind]);
```
so I think having another loop should be fine.


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:930
   };
-  // If code never references data variables (the symbol is unneeded), and
-  // linker GC cannot discard data variables while the text section is retained,
-  // data variables can be private. This optimization applies on COFF and ELF.
-  if (!DataReferencedByCode && !TT.isOSBinFormatMachO()) {
+  // If the data variable is not referenced by code (when NS==0, i.e. no value
+  // profiling), and the counter keeps the data variable live under linker GC,
----------------
davidxl wrote:
> What is the root cause of the failure when NS == 0 condition is not used?
5798be84580be233e4cf34c08ceec8f79e80502e has the message.

Say a.cc has a prevailing comdat and b.cc's is non-prevailing.

b.cc's linkonce_odr comdat function may get inlined into a function not in a.cc.
It is a linker error for the prevailing function to reference a discarded STB_LOCAL symbol (`__profd_` is non-prevailing and thus discarded)

If we don't change the linkage, `__profd_` is STB_GLOBAL STV_HIDDEN. It is fine to reference such a non-local symbol, which will bind to a.cc's copy.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103717/new/

https://reviews.llvm.org/D103717