This is an archive of the discontinued LLVM Phabricator instance.

PGO] Fix __llvm_profile_raw_version linkage in MACHO
ClosedPublic

Authored by xur on May 9 2016, 1:16 PM.

Details

Summary

IR instrumentation generates a COMDAT symbol __llvm_profile_raw_version to overwrite the same symbol in profile run-time to distinguish IR profiles from Clang generated profiles. In MACHO, LinkOnceODR linkage is used due to the lack of COMDAT support.

But LinkOnceODR linkage might have .weak_def_can_be_hidden assembly directive, while the weak variable in run-time has a .weak_definition directive. Linker will not merge these two symbols even they have the same name. The end result is IR profiles are not properly flagged in MACHO.

This patch changes the linkage for __llvm_profile_raw_version in each module to LinkOnceAny so that it has same .weak_definition directive as in the run-time..

Diff Detail

Event Timeline

xur updated this revision to Diff 56616.May 9 2016, 1:16 PM
xur retitled this revision from to PGO] Fix __llvm_profile_raw_version linkage in MACHO.
xur updated this object.
xur added reviewers: vsk, davidxl.
xur added subscribers: llvm-commits, xur.
vsk edited edge metadata.May 9 2016, 1:28 PM

Wow, so without this fix are IR profiles unusable on macho? I'm not sure if I'm understanding this correctly because it doesn't seem like that could pass testing.

Also, is "weak_def_can_be_hidden" incorrect because __llvm_profile_raw_version isn't supposed to be "hidden"? I'd appreciate it if you could point me to any resources that discuss the difference between weak_def_can_be_hidden and weak_definition -- if there aren't any I can dig a bit more :).

xur added a comment.May 9 2016, 1:50 PM
In D20078#425034, @vsk wrote:

Wow, so without this fix are IR profiles unusable on macho? I'm not sure if I'm understanding this correctly because it doesn't seem like that could pass testing.

Unfortunately, this is true: IR profiles is broken in MACHO for profile-use. We don't have an end-to-end PGO test for IR profiles. I will add later.

Also, is "weak_def_can_be_hidden" incorrect because __llvm_profile_raw_version isn't supposed to be "hidden"? I'd appreciate it if you could point me to any resources that discuss the difference between weak_def_can_be_hidden and weak_definition -- if there aren't any I can dig a bit more :).

Our tests show that what attribute (weak_def_can_be_hidden or weak_definition) to use does not seem to matter --- we just cannot mix this two. I chose weak_definition because it's easier to write the code in the profile run-time (just use attribute((weak)) as it's). It's harder to write code to generate the weak_def_can_be_hidden attribute.

We thought apple has better documentation about weak_def_can_be_hidden because this seems to be macho specific. I only find a few references online. Here is one:
 http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20131028/193223.html

vsk accepted this revision.May 9 2016, 1:56 PM
vsk edited edge metadata.

Our tests show that what attribute (weak_def_can_be_hidden or weak_definition) to use does not seem to matter --- we just cannot mix this two. I chose weak_definition because it's easier to write the code in the profile run-time (just use attribute((weak)) as it's). It's harder to write code to generate the weak_def_can_be_hidden attribute.

Got it. According to that commit, weak_def_can_be_hidden is appropriate if the symbol's address isn't taken. It seems like .weak_definition is more suitable here, then.

This revision is now accepted and ready to land.May 9 2016, 1:56 PM
This revision was automatically updated to reflect the committed changes.