[InstrProfiling] Use external weak reference for bias variable

[phosek]

Summary

We need the compiler generated variable to override the weak symbol of
the same name inside the profile runtime, but using LinkOnceODRLinkage
results in weak symbol being emitted in which case the symbol selected
by the linker is going to depend on the order of inputs which is far too
fragile.

This change replaces the use of weak definition inside the runtime with
an external weak reference to address the issue.

We also place the compiler generated symbol inside a COMDAT group so
dead definition can be garbage collected by the linker.

We also disable the use of runtime counter relocation on Darwin since
Mach-O doesn't support weak external references, but Darwin already uses
a different continous mode that relies on overmapping so runtime counter
relocation isn't needed there.

[mcgrathr]

It makes sense that the contract be that the runtime must define the variable.

I don't understand how this change is accomplishing that, however. From the IR in the test case, it's become a hidden global definition in a COMDAT group. AFAIK, that's what linkonce_odr actually translates to in ELF backends anyway. What's the difference in the .s or .o file generated from that IR? I would expect to just use a vanilla undefined external symbol here.

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

693

This merits a comment about the contract, e.g. "The runtime must define this variable."

701

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.

[davidxl]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

701

The variable is defined (it has an initialization).

The related question is that without COMDAT support, would this leads to multiple definition linker error?

[phosek]

After an offline discussion with @mcgrathr we've decided to switch to external weak reference for the bias variable which is a cleaner solution.

[davidxl]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

700

Is this change still needed?

[phosek]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

700

It's not necessary for correctness, but should enable GC in the case this variable ends up unused.

[davidxl]

lgtm

[mcgrathr]

lgtm modulo some comment cleanups

compiler-rt/lib/profile/InstrProfiling.h

324–325

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.

compiler-rt/lib/profile/InstrProfilingFile.c

1002–1004

Any check of a weak reference like this merits a comment since it's always a subtle matter.

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

701

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.

[thakis]

This breaks 52 libprofile tests on macOS: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8842844620347296624/+/u/package_clang/stdout?format=raw

Undefined symbols for architecture x86_64:
  "___llvm_profile_counter_bias", referenced from:
      ___llvm_profile_initialize_file in libclang_rt.profile_osx.a(InstrProfilingFile.c.o)
      ___llvm_profile_initialize in libclang_rt.profile_osx.a(InstrProfilingFile.c.o)
      _relocateCounters in libclang_rt.profile_osx.a(InstrProfilingFile.c.o)
ld: symbol(s) not found for architecture x86_64
clang-13: error: linker command failed with exit code 1 (use -v to see invocation)

I reverted this in a92964779cb5fa59e832816b14a30bc8dbf927a9 for now.

[thakis]

(I believe @MaskRay did a lot of cool work in this general area recently, maybe he knows how to make this work.)

[MaskRay]

In D105176#2855238, @thakis wrote:

(I believe @MaskRay did a lot of cool work in this general area recently, maybe he knows how to make this work.)

This is the normal problem that ld64 disallows undefined symbols by default, even in dylib mode.
To make the linker happy, we can use a dynamic lookup symbol, i.e. -Wl,-U,__llvm_profile_counter_bias.
However, this just defers the error to the runtime dyld: Symbol not found.

Am I right that Mach-O doesn't really support undefined weak symbols?
(The binary format is unfortunate in this case: the hidden state of an undefined weak is not represented, so the linker cannot know the symbol needs to be resolved to zero)

Perhaps disallowing this for Darwin or making this Fuchsia specific is the simplest solution.

For

extern int var;
void *addr() { return &var; }

-fprofile-instr-generate -fcoverage-mapping a.c -S -O2 -mllvm -runtime-counter-relocation -fno-asynchronous-unwind-tables -fno-exceptions produces:

addr:                                   # @addr
# %bb.0:                                # %entry
        movq    __llvm_profile_counter_bias(%rip), %rax
        addq    $1, .L__profc_addr(%rax)
        movl    $var, %eax
        retq

The indirection is unfortunate. I don't think Linux users may want to have this.

[mcgrathr]

MaskRay's code generation example shows the optimal code possible for implementing this feature and how that code is generated is unaffected by this change, so I don't understand why that subject is being raised here.

As to the linking issues, it may make sense to define things differently for different object formats. Using the approach taken here for ELF platforms and leaving other platforms to their own devices seems like a sensible plan to me.

[MaskRay]

If an undefined weak symbol is kept, restricting to ELF only looks fine to me.

but using LinkOnceODRLinkage results in weak symbol being emitted which leads to an issue where the linker might choose either of the weak symbols potentially disabling the runtime counter relocation.

But I don't understand this sentence from the description. Are you linking libclang_rt.profile-* before other input files?
If there may be conflicting definitions, LinkOnceODRLinkage is not suitable, but you can use LinkOnceAnyLinkage/WeakAnyLinkage

[phosek]

I'm fine disabling this for Mach-O which uses the continuous mode that relies on overmapping (see https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#running-the-instrumented-program for more details).

I'll clarify the commit message, the issue is that relying on link order is fragile and can lead to hard-to-notice issues which we observed in practice.

[MaskRay]

We also disable the use of runtime counter relocation on Darwin since Mach-O doesn't support weak external references, but Darwin already uses a different continous mode that relies on overmapping so runtime counter relocation isn't needed there.

I think you can restrict undefined weak to ELF now. PE-COFF doesn't have undefined weak, either.

[phosek]

In D105176#2865370, @MaskRay wrote:

We also disable the use of runtime counter relocation on Darwin since Mach-O doesn't support weak external references, but Darwin already uses a different continous mode that relies on overmapping so runtime counter relocation isn't needed there.

I think you can restrict undefined weak to ELF now. PE-COFF doesn't have undefined weak, either.

Done, do you know if there's any alternative we could use for PE-COFF?

[MaskRay]

In D105176#2865477, @phosek wrote:

In D105176#2865370, @MaskRay wrote:

We also disable the use of runtime counter relocation on Darwin since Mach-O doesn't support weak external references, but Darwin already uses a different continous mode that relies on overmapping so runtime counter relocation isn't needed there.

I think you can restrict undefined weak to ELF now. PE-COFF doesn't have undefined weak, either.

Done, do you know if there's any alternative we could use for PE-COFF?

A comdat any is the easiest. The runtime defines a comdat any variable with value 0 which can be overridden by the program.

It does rely on the link order but is the easiest.

Alternatives include IMAGE_SYM_CLASS_WEAK_EXTERNAL (weak definition) and linker directives /alternative: both of which are quite different/inconvenient.

So I want to re-ask how the order difference is a problem. Using a comdat any can be straightforwardly ported to PE/COFF.

I'll clarify the commit message, the issue is that relying on link order is fragile and can lead to hard-to-notice issues which we observed in practice.

[phosek]

In D105176#2865512, @MaskRay wrote:

In D105176#2865477, @phosek wrote:

In D105176#2865370, @MaskRay wrote:

We also disable the use of runtime counter relocation on Darwin since Mach-O doesn't support weak external references, but Darwin already uses a different continous mode that relies on overmapping so runtime counter relocation isn't needed there.

I think you can restrict undefined weak to ELF now. PE-COFF doesn't have undefined weak, either.

Done, do you know if there's any alternative we could use for PE-COFF?

A comdat any is the easiest. The runtime defines a comdat any variable with value 0 which can be overridden by the program.

We need to distinguish the case where the variable is defined by the TU (in which case it has to be initialized with value 0) and where the variable is defined by the runtime.

Previously, the variable defined by the runtime would have the value -1 and the variable defined by the TU would have 0. After this change, runtime has an undefined reference and variable defined by TU is still initialized to 0.

Given that requirement, I'm not sure comdat any is going to work.

It does rely on the link order but is the easiest.

Alternatives include IMAGE_SYM_CLASS_WEAK_EXTERNAL (weak definition) and linker directives /alternative: both of which are quite different/inconvenient.

That might work, I'll look into it.

So I want to re-ask how the order difference is a problem. Using a comdat any can be straightforwardly ported to PE/COFF.

I noticed that in some of our instrumented binaries, the runtime definition initialized to -1 prevails in the binary. I haven't had pinpoint exactly why it's happening, in some cases we do mix C++ and Rust and manually link the profile runtime and I suspect that this might be one of the culprits.

I can dig into this a bit more and try to find, but I still think that relying on link order is fragile (especially since you won't notice the issue until you run the binary) and so I'd like to avoid it if possible.

I'll clarify the commit message, the issue is that relying on link order is fragile and can lead to hard-to-notice issues which we observed in practice.

[phosek]

@MaskRay it looks like we could use the following pattern:

extern intptr_t __llvm_profile_counter_bias;

#if defined(_WIN32)
intptr_t __llvm_profile_counter_bias_default = 0;
#pragma comment(linker, "/alternatename:" \
                SYMBOL_NAME("__llvm_profile_counter_bias") "=" \
                SYMBOL_NAME("__llvm_profile_counter_bias_default"))
#endif

bool usesRuntimeCounterRelocation() {
  void *s1 = &__llvm_profile_counter_bias;
  void *s2 = &__llvm_profile_counter_bias_default;
  return s1 != s2;
}

This does seem to work with clang-cl but when I tried this with cl I still get an error:

error LNK2019: unresolved external symbol __llvm_profile_counter_bias referenced in function usesRuntimeCounterRelocation

[phosek]

In D105176#2868626, @phosek wrote:

@MaskRay it looks like we could use the following pattern:

extern intptr_t __llvm_profile_counter_bias;

#if defined(_WIN32)
intptr_t __llvm_profile_counter_bias_default = 0;
#pragma comment(linker, "/alternatename:" \
                SYMBOL_NAME("__llvm_profile_counter_bias") "=" \
                SYMBOL_NAME("__llvm_profile_counter_bias_default"))
#endif

bool usesRuntimeCounterRelocation() {
  void *s1 = &__llvm_profile_counter_bias;
  void *s2 = &__llvm_profile_counter_bias_default;
  return s1 != s2;
}

This does seem to work with clang-cl but when I tried this with cl I still get an error:

error LNK2019: unresolved external symbol __llvm_profile_counter_bias referenced in function usesRuntimeCounterRelocation

Never mind, I have figured it out. This should now handle COFF as well.

[thakis]

compiler-rt/lib/profile/InstrProfilingFile.c

456

../../compiler-rt/lib/profile/InstrProfilingFile.c:429:12: warning: unused function 'writeMMappedFile' [-Wunused-function]
static int writeMMappedFile(FILE *OutputFile, char **Profile) {
           ^

I guess this should now just move into the elif?

compiler-rt/lib/profile/CMakeLists.txt

 
 add_compiler_rt_component(profile)
 
 set(PROFILE_SOURCES
   GCDAProfiling.c
   InstrProfiling.c
   InstrProfilingInternal.c
   InstrProfilingValue.c
-  InstrProfilingBiasVar.c
   InstrProfilingBuffer.c
   InstrProfilingFile.c
   InstrProfilingMerge.c
   InstrProfilingMergeFile.c
   InstrProfilingNameVar.c
   InstrProfilingVersionVar.c
   InstrProfilingWriter.c
   InstrProfilingPlatformDarwin.c

compiler-rt/lib/profile/InstrProfiling.h

 /*!
  * This variable is a weak symbol defined in InstrProfiling.c. It allows
  * compiler instrumentation to provide overriding definition with value
  * from compiler command line. This variable has default visibility.
  */
 extern char INSTR_PROF_PROFILE_NAME_VAR[1]; /* __llvm_profile_filename. */
 
 /*!
- * This variable is a weak symbol defined in InstrProfilingBiasVar.c. It
- * allows compiler instrumentation to provide overriding definition with
- * value from compiler command line. This variable has hidden visibility.
+ * This variable is a weak external reference which could be used to detect
+ * whether or not the compiler defined this symbol.
  */
-COMPILER_RT_VISIBILITY extern intptr_t __llvm_profile_counter_bias;
+COMPILER_RT_VISIBILITY COMPILER_RT_WEAK extern intptr_t
+    __llvm_profile_counter_bias;
 
 #endif /* PROFILE_INSTRPROFILING_H_ */

compiler-rt/lib/profile/InstrProfilingBiasVar.c

-/*===- InstrProfilingBiasVar.c - profile counter bias variable setup ------===*\
-|*
-|* Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-|* See https://llvm.org/LICENSE.txt for license information.
-|* SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-|*
-\*===----------------------------------------------------------------------===*/
-
-#include "InstrProfiling.h"
-
-/* The runtime should only provide its own definition of this symbol when the
- * user has not specified one. Set this up by moving the runtime's copy of this
- * symbol to an object file within the archive.
- */
-COMPILER_RT_WEAK intptr_t __llvm_profile_counter_bias = -1;

compiler-rt/lib/profile/InstrProfilingFile.c

static void unlockProfile(int *ProfileRequiresUnlock, FILE *File) {
   if (!*ProfileRequiresUnlock) {
     PROF_WARN("%s", "Expected to require profile unlock\n");
   }
   lprofUnlockFileHandle(File);
   *ProfileRequiresUnlock = 0;
 }
 #endif // !defined(__Fuchsia__) && !defined(_WIN32)
 
 static int writeMMappedFile(FILE *OutputFile, char **Profile) {
   if (!OutputFile)
     return -1;
 
   /* Write the data into a file. */
   setupIOBuffer();
   ProfDataWriter fileWriter;
   initFileWriter(&fileWriter, OutputFile);
   if (lprofWriteData(&fileWriter, NULL, 0)) {

  */
 COMPILER_RT_VISIBILITY
 void __llvm_profile_initialize_file(void) {
   const char *EnvFilenamePat;
   const char *SelectedPat = NULL;
   ProfileNameSpecifier PNS = PNS_unknown;
   int hasCommandLineOverrider = (INSTR_PROF_PROFILE_NAME_VAR[0] != 0);
 
-  if (__llvm_profile_counter_bias != -1)
+  /* This symbol is defined by the compiler when runtime counter relocation is
+   * used and runtime provides a weak external reference so we can check it. */
+  if (&__llvm_profile_counter_bias)
     lprofSetRuntimeCounterRelocation(1);
 
   EnvFilenamePat = getFilenamePatFromEnv();
   if (EnvFilenamePat) {
     /* Pass CopyFilenamePat = 1, to ensure that the filename would be valid
        at the  moment when __llvm_profile_write_file() gets executed. */
     parseAndSetFilename(EnvFilenamePat, PNS_environment, 1);
     return;

compiler-rt/lib/profile/InstrProfilingPlatformFuchsia.c

void __llvm_profile_initialize(void) {
   if (GET_VERSION(__llvm_profile_get_version()) != INSTR_PROF_RAW_VERSION) {
     lprofWrite("LLVM Profile: runtime and instrumentation version mismatch: "
                "expected %d, but got %d\n",
                INSTR_PROF_RAW_VERSION,
                (int)GET_VERSION(__llvm_profile_get_version()));
     return;
   }
 
-  /* This symbol is defined as weak and initialized to -1 by the runtimer, but
-   * compiler will generate a strong definition initialized to 0 when runtime
-   * counter relocation is used. */
-  if (__llvm_profile_counter_bias == -1) {
+  /* This symbol is defined by the compiler when runtime counter relocation is
+   * used and runtime provides a weak external reference so we can check it. */
+  if (!&__llvm_profile_counter_bias) {
     lprofWrite("LLVM Profile: counter relocation at runtime is required\n");
     return;
   }
 
   const __llvm_profile_data *DataBegin = __llvm_profile_begin_data();
   const __llvm_profile_data *DataEnd = __llvm_profile_end_data();
   const uint64_t *CountersBegin = __llvm_profile_begin_counters();
   const uint64_t *CountersEnd = __llvm_profile_end_counters();

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

if (isRuntimeCounterRelocationEnabled()) {
     Function *Fn = Inc->getParent()->getParent();
     Instruction &I = Fn->getEntryBlock().front();
     LoadInst *LI = dyn_cast<LoadInst>(&I);
     if (!LI) {
       IRBuilder<> Builder(&I);
       Type *Int64Ty = Type::getInt64Ty(M->getContext());
       GlobalVariable *Bias = M->getGlobalVariable(getInstrProfCounterBiasVarName());
       if (!Bias) {
+        // Compiler must define this variable when runtime counter relocation
+        // is being used. Runtime has a weak external reference that is used
+        // to check whether that's the case or not.
         Bias = new GlobalVariable(*M, Int64Ty, false, GlobalValue::LinkOnceODRLinkage,
                                   Constant::getNullValue(Int64Ty),
                                   getInstrProfCounterBiasVarName());
         Bias->setVisibility(GlobalVariable::HiddenVisibility);
+        // 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.
+        if (TT.supportsCOMDAT())
+            Bias->setComdat(M->getOrInsertComdat(Bias->getName()));
       }
       LI = Builder.CreateLoad(Int64Ty, Bias);
     }
     auto *Add = Builder.CreateAdd(Builder.CreatePtrToInt(Addr, Int64Ty), LI);
     Addr = Builder.CreateIntToPtr(Add, Int64PtrTy);
   }
 
   if (Options.Atomic || AtomicCounterUpdateAll ||

llvm/test/Instrumentation/InstrProfiling/runtime-counter-relocation.ll

 ; RUN: opt < %s -S -instrprof | FileCheck %s
 ; RUN: opt < %s -S -instrprof -runtime-counter-relocation | FileCheck -check-prefixes=RELOC %s
 
 target triple = "x86_64-unknown-linux-gnu"
 
 @__profn_foo = private constant [3 x i8] c"foo"
-; RELOC: @__llvm_profile_counter_bias = linkonce_odr hidden global i64 0
+; RELOC: $__llvm_profile_counter_bias = comdat any
+; RELOC: @__llvm_profile_counter_bias = linkonce_odr hidden global i64 0, comdat
 
 ; CHECK-LABEL: define void @foo
 ; CHECK-NEXT: %pgocount = load i64, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc_foo, i64 0, i64 0)
 ; CHECK-NEXT: %1 = add i64 %pgocount, 1
 ; CHECK-NEXT: store i64 %1, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc_foo, i64 0, i64 0)
 ; RELOC-LABEL: define void @foo
 ; RELOC-NEXT: %1 = load i64, i64* @__llvm_profile_counter_bias
 ; RELOC-NEXT: %2 = add i64 ptrtoint ([1 x i64]* @__profc_foo to i64), %1