[PATCH] D105176: [InstrProfiling] Use external weak reference for bias variable

[Roland McGrath via Phabricator via llvm-commits]

mcgrathr accepted this revision.
mcgrathr added a comment.

lgtm modulo some comment cleanups



================
Comment at: compiler-rt/lib/profile/InstrProfiling.h:324-325
+ * This variable is a weak symbol defined by the compiler when the runtime
+ * counter relocation is enabled. Set this up by moving the runtime's copy
+ * of this symbol to an object file within the archive.
  */
----------------
I don't understand this sentence.
The runtime has no "copy" of this symbol, it only refers to it.
The salient point here is that this is a weak undefined reference so the runtime can detect whether or not the compiler defined the symbol.



================
Comment at: compiler-rt/lib/profile/InstrProfilingFile.c:1002
 
-  if (__llvm_profile_counter_bias != -1)
+  if (&__llvm_profile_counter_bias)
     lprofSetRuntimeCounterRelocation(1);
----------------
Any check of a weak reference like this merits a comment since it's always a subtle matter.



================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:698
+        if (TT.supportsCOMDAT())
+            Bias->setComdat(M->getOrInsertComdat(Bias->getName()));
       }
----------------
davidxl wrote:
> mcgrathr wrote:
> > What's the meaning of comdat on an undefined external?
> > There is no such concept at the object file level, at least not in ELF.
> > 
> The variable is defined (it has an initialization).
> 
> The related question is that without COMDAT support, would this leads to multiple definition linker error?
A definition that's weak (linkonce_odr) without being in a comdat section wouldn't lead to link errors, but it would lead to a dead data word from every TU but one.   Putting it in comdat ensures there will be exactly one data slot in the link.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105176