This is an archive of the discontinued LLVM Phabricator instance.

[InstrProfiling] Use external weak reference for bias variable
ClosedPublic

Authored by phosek on Jun 30 2021, 12:23 AM.

Details

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.

Diff Detail

Event Timeline

phosek created this revision.Jun 30 2021, 12:23 AM
phosek requested review of this revision.Jun 30 2021, 12:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2021, 12:23 AM

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 added inline comments.Jun 30 2021, 4:41 PM
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 updated this revision to Diff 355734.Jun 30 2021, 5:28 PM
phosek retitled this revision from [InstrProfiling] Use external linkage for bias variable to [InstrProfiling] Use external weak reference for bias variable.
phosek edited the summary of this revision. (Show Details)

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

Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2021, 5:28 PM
Herald added subscribers: Restricted Project, mgorny. · View Herald Transcript
davidxl added inline comments.Jun 30 2021, 6:44 PM
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
700

Is this change still needed?

phosek added inline comments.Jun 30 2021, 8:24 PM
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 accepted this revision.Jun 30 2021, 9:58 PM

lgtm

This revision is now accepted and ready to land.Jun 30 2021, 9:58 PM
phosek updated this revision to Diff 356021.Jul 1 2021, 2:48 PM
mcgrathr accepted this revision.Jul 1 2021, 3:01 PM

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.

phosek updated this revision to Diff 356029.Jul 1 2021, 3:23 PM
phosek marked 3 inline comments as done.
This revision was landed with ongoing or failed builds.Jul 1 2021, 3:26 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jul 2 2021, 6:05 AM

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 added a subscriber: MaskRay.Jul 2 2021, 6:06 AM

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

MaskRay added a comment.EditedJul 2 2021, 7:40 PM

(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.

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 added a comment.EditedJul 5 2021, 3:32 PM

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 added a comment.Jul 7 2021, 2:29 PM

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.

phosek updated this revision to Diff 357153.Jul 8 2021, 12:38 AM
phosek edited the summary of this revision. (Show Details)
phosek reopened this revision.Jul 8 2021, 12:26 PM
This revision is now accepted and ready to land.Jul 8 2021, 12:26 PM

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 updated this revision to Diff 357345.Jul 8 2021, 1:47 PM

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 added a comment.EditedJul 8 2021, 2:01 PM

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 added a comment.Jul 8 2021, 3:18 PM

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 added a comment.Jul 9 2021, 5:54 PM

@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 updated this revision to Diff 358162.Jul 12 2021, 10:55 PM

@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.

phosek updated this revision to Diff 359866.Jul 19 2021, 11:54 AM
This revision was landed with ongoing or failed builds.Jul 19 2021, 12:24 PM
This revision was automatically updated to reflect the committed changes.
thakis added inline comments.Jul 19 2021, 4:46 PM
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/InstrProfiling.h