Page MenuHomePhabricator

Reland [pgo] Avoid introducing relocations by using private alias
ClosedPublic

Authored by paulkirth on Nov 14 2022, 1:57 PM.

Details

Summary

In many cases, we can use an alias to avoid a symbolic relocations,
instead of using the public, interposable symbol. When the instrumented
function is in a COMDAT, we can use a hidden alias, and still avoid
references to discarded sections.

Previous versions of this patch allowed the compiler to name the
generated alias, but that would only be valid when the functions were
local. Since the alias may be used across TUs we use a more
deterministic naming convention, and add a ".local" suffix to the alias
name just as we do for relative vtables aliases.

https://reviews.llvm.org/rG20894a478da224bdd69c91a22a5175b28bc08ed9
removed an incorrect assertion on Mach-O which caused assertion failures in LLD.

We prevent duplicate symbols under ThinLTO + PGO + CFI by disabling
alias generation when the target function has MD_type metadata used in
CFI.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Update summary

This revision was landed with ongoing or failed builds.Dec 8 2022, 1:03 PM
This revision was automatically updated to reflect the committed changes.

@steven_wu Sorry about that and thanks for the report. I thought I had addressed all the platform inconsistencies with that test when I disabled Windows. I will just move it under the Linux tests to be safe.

paulkirth reopened this revision.Dec 8 2022, 4:28 PM
This revision is now accepted and ready to land.Dec 8 2022, 4:28 PM
paulkirth updated this revision to Diff 481473.Dec 8 2022, 4:36 PM
paulkirth edited the summary of this revision. (Show Details)

Move the new runtime test under Linux, and ensure it only runs on that platform

paulkirth updated this revision to Diff 481485.Dec 8 2022, 5:27 PM

Move the failing test under the Linux folder and restrict it to only run on Linux

This revision was landed with ongoing or failed builds.Dec 8 2022, 5:28 PM
This revision was automatically updated to reflect the committed changes.

The errors from the comment https://reviews.llvm.org/D137982#3929427 are back after the relanding.

@glandium can you provide more information on your build? I specifically tested this using the directions you provided, and build an IR instrumented clang just fine.

paulkirth reopened this revision.Dec 8 2022, 8:34 PM
This revision is now accepted and ready to land.Dec 8 2022, 8:34 PM

I can reproduce with this:

cmake -G Ninja -S llvm -B stage1 -DLLVM_LINK_LLVM_DYLIB=ON -DLLVM_ENABLE_PROJECTS=clang -DCMAKE_BUILD_TYPE=Release
ninja -C stage1
cmake -G Ninja -S llvm -B stage2 -DLLVM_LINK_LLVM_DYLIB=ON -DLLVM_ENABLE_PROJECTS=clang -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=$PWD/stage1/bin/clang -DLLVM_BUILD_INSTRUMENTED=IR -DCMAKE_SHARED_LINKER_FLAGS="-fuse-ld=gold"
ninja -C stage2

The key is to be using GNU gold.

paulkirth updated this revision to Diff 481808.Dec 9 2022, 5:37 PM
paulkirth edited the summary of this revision. (Show Details)

Rebase and use deterministic naming for generated alias.

Allowing the compiler to generate a name if fine for local/private aliases, but
some of these symbols can have linkages that may result in global visibility,
so we should use a deterministic naming convention, so that they can match
correctly.

paulkirth added a comment.EditedDec 9 2022, 5:44 PM

@glandium, I've tested this locally, and fixing the alias naming seems to satisfy gold when instrumenting clang w/ llvm as a dylib. lld also seems to be working fine.

paulkirth updated this revision to Diff 483682.Dec 16 2022, 3:10 PM

Rebase

still trying to determine if presubmit checks are related to this patch, since they relate to profiling

This revision was landed with ongoing or failed builds.Dec 19 2022, 10:46 AM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Dec 21 2022, 6:28 AM

This caused lld on mac to assert when building instrumented clang (or
instrumented code in general):

$ export SDKROOT=$(xcrun --show-sdk-path)
$ cat /tmp/a.cc
#include <cerrno>
#include <iostream>
#include <string>
#include <system_error>

std::string getMessageFor(int err) {
    return std::make_error_code(static_cast<std::errc>(err)).message();
}

int main() {
    std::cout << getMessageFor(ENOENT) << ';' << getMessageFor(EISDIR);
    std::cout << ';' << getMessageFor(EINVAL) << ';' << getMessageFor(EACCES);
}
$ build/bin/clang++ /tmp/a.cc -fprofile-generate=/tmp/foo -fuse-ld=lld -O3
Assertion failed: (!isWeakDefCanBeHidden && "weak_def_can_be_hidden on already-hidden symbol?"), function createDefined, file InputFiles.cpp, line 699.

I'll revert it for now.

paulkirth reopened this revision.Dec 21 2022, 2:02 PM

@hans Thanks for the revert, and sorry for the trouble. I 'll take a look at your reproducer and see if I can work around those issues.

This revision is now accepted and ready to land.Dec 21 2022, 2:02 PM

@hans can you supply some more information regarding your configuration? I'm having trouble reproducing this failure when I build on Mac. Also, is this on Apple silicon or Intel?

I'm able to test on an Intel MacBook Pro 2019 using the following commands with your example program.
Cmake invocation:

cmake -G Ninja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;lld" -DLLVM_ENABLE_RUNTIMES="compiler-rt" ~/llvm-project/llvm -DBUILTINS_CMAKE_ARGS=-DCOMPILER_RT_ENABLE_IOS=OFF

Compilation command:

SDKROOT=$(xcrun --show-sdk-path) bin/clang++ /tmp/a.cc -fprofile-generate=/tmp/foo -fuse-ld=lld -O3 -v -L $PWD/lib

I can run the executable and generate a profile with it, and recompile using that profile and I get no assertions from lld and no runtime errors...

paulkirth updated this revision to Diff 487542.Mon, Jan 9, 1:21 PM
paulkirth retitled this revision from Reland "[pgo] Avoid introducing relocations by using private alias" to "Reland "[pgo] Avoid introducing relocations by using private alias".
paulkirth edited the summary of this revision. (Show Details)

Rebase.

  • move test from linux only to also run on mac
  • update summary
paulkirth updated this revision to Diff 487585.Mon, Jan 9, 3:36 PM

Rebase correctly

This revision was landed with ongoing or failed builds.Wed, Jan 11, 1:53 PM
This revision was automatically updated to reflect the committed changes.

fyi this is causing a link error in PGO instrumented builds: https://bugs.chromium.org/p/chromium/issues/detail?id=1408161, investigating

@aeubanks Thanks for the report. Is there a reproducer for this? I don't' see one on the CI bot, and I'd like to understand how this happens.

Shall we revert this now, or should we dig in some more first?

@aeubanks Thanks for the report. Is there a reproducer for this? I don't' see one on the CI bot, and I'd like to understand how this happens.

Shall we revert this now, or should we dig in some more first?

More details in the bug. Seems to be some combination of building libc++ from source, CFI (which requires ThinLTO), and PGO instrumentation. I'll revert now.

paulkirth reopened this revision.Tue, Jan 17, 3:52 PM

@aeubanks Thanks for the revert. I'll be digging into this as well.

This revision is now accepted and ready to land.Tue, Jan 17, 3:52 PM
paulkirth updated this revision to Diff 490309.Wed, Jan 18, 3:02 PM
paulkirth retitled this revision from "Reland "[pgo] Avoid introducing relocations by using private alias" to Reland [pgo] Avoid introducing relocations by using private alias.
paulkirth edited the summary of this revision. (Show Details)

Rebase.

resolves the issue I was seeing, so no objections from me

paulkirth reopened this revision.Thu, Jan 19, 12:54 PM

CFI + ThinLTO + PGO is still an issue, so we need to figure out why duplicate symbols are getting generated, despite our other changes.

This revision is now accepted and ready to land.Thu, Jan 19, 12:54 PM
paulkirth updated this revision to Diff 491928.Tue, Jan 24, 3:02 PM

Don't create an alias when type metadata is present. We need a more thoughtful
solution for the ThinLTO + CFI case, since we would need to move any generated
aliases into the main module w/ the jump tables.

That will require more complex changes to some combination of CFI and PGO, and
is sufficiently complex to warrant its own patch. In the meantime we can still
benefit from generating the alias in most cases.

paulkirth edited the summary of this revision. (Show Details)

Rebase and update summary.