This is an archive of the discontinued LLVM Phabricator instance.

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
paulkirth updated this revision to Diff 475270.Nov 14 2022, 1:58 PM

Fix formatting

phosek added a subscriber: phosek.Nov 14 2022, 10:46 PM
phosek added inline comments.
llvm/test/Transforms/PGOProfile/prof_avoid_relocs.ll
26

Does this check anything beyond what's already checked by my_foo, my_bar and my_baz below? Could we just a single function (e.g. `my_foo) to simplify this test?

Simplify test case.

We only need to check that the alias is used in the __profd_foo and that it
doesn't use the public symbol, so this should be enough.

Add sanity checks for the instrumentation in the function body.

paulkirth marked an inline comment as done.Nov 14 2022, 11:45 PM
paulkirth added inline comments.
llvm/test/Transforms/PGOProfile/prof_avoid_relocs.ll
26

That's a good point. I've simplified the test significantly. I added a sanity check for the function body that its instrumented as expected, but that could be removed too.

phosek accepted this revision.Nov 15 2022, 12:07 AM

LGTM

This revision is now accepted and ready to land.Nov 15 2022, 12:07 AM
This revision was automatically updated to reflect the committed changes.
paulkirth marked an inline comment as done.

This broke (at least) building clang itself with PGO (note this is using the gold linker, not lld):

ib/libLLVMObject.a(ELF.cpp.o):ELF.cpp:__profd__ZNK4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE14getStringTableERKNS0_13Elf_Shdr_ImplIS5_EENS_12function_refIFNS_5ErrorERKNS_5TwineEEEE: error: relocation refers to local symbol "" [534], which is defined in a discarded section
  section group signature: "_ZNK4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE14getStringTableERKNS0_13Elf_Shdr_ImplIS5_EENS_12function_refIFNS_5ErrorERKNS_5TwineEEEE"
  prevailing definition is from lib/libLLVMRuntimeDyld.a(RuntimeDyldELF.cpp.o)
lib/libLLVMObject.a(ELF.cpp.o):ELF.cpp:__profd__ZNK4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE8sectionsEv: error: relocation refers to local symbol "" [551], which is defined in a discarded section
  section group signature: "_ZNK4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE8sectionsEv"
  prevailing definition is from lib/libLLVMRuntimeDyld.a(RuntimeDyldELF.cpp.o)
lib/libLLVMObject.a(ELF.cpp.o):ELF.cpp:__profd__ZNK4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE10getSectionEj: error: relocation refers to local symbol "" [560], which is defined in a discarded section
  section group signature: "_ZNK4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE10getSectionEj"
  prevailing definition is from lib/libLLVMRuntimeDyld.a(RuntimeDyldELF.cpp.o)
lib/libLLVMObject.a(ELF.cpp.o):ELF.cpp:__profd__ZNK4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE13getSHNDXTableERKNS0_13Elf_Shdr_ImplIS5_EE: error: relocation refers to local symbol "" [561], which is defined in a discarded section
  section group signature: "_ZNK4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE13getSHNDXTableERKNS0_13Elf_Shdr_ImplIS5_EE"
  prevailing definition is from lib/libLLVMRuntimeDyld.a(RuntimeDyldELF.cpp.o)
lib/libLLVMObject.a(ELF.cpp.o):ELF.cpp:__profd__ZNK4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE13getSHNDXTableERKNS0_13Elf_Shdr_ImplIS5_EENS_8ArrayRefIS8_EE: error: relocation refers to local symbol "" [562], which is defined in a discarded section
  section group signature: "_ZNK4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE13getSHNDXTableERKNS0_13Elf_Shdr_ImplIS5_EENS_8ArrayRefIS8_EE"
  prevailing definition is from lib/libLLVMRuntimeDyld.a(RuntimeDyldELF.cpp.o)
lib/libLLVMObject.a(ELF.cpp.o):ELF.cpp:__profd__ZNK4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE21getRelocationTypeNameEjRNS_15SmallVectorImplIcEE: error: relocation refers to local symbol "" [590], which is defined in a discarded section 
  section group signature: "_ZNK4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE21getRelocationTypeNameEjRNS_15SmallVectorImplIcEE"
  prevailing definition is from lib/libLLVMRuntimeDyld.a(RuntimeDyldELF.cpp.o)
lib/libLLVMObject.a(ELF.cpp.o):ELF.cpp:__profd__ZN4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE6createENS_9StringRefE: error: relocation refers to local symbol "" [713], which is defined in a discarded section
  section group signature: "_ZN4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE6createENS_9StringRefE"
  prevailing definition is from lib/libLLVMRuntimeDyld.a(RuntimeDyldELF.cpp.o)
lib/libLLVMObject.a(ELF.cpp.o):ELF.cpp:__profd__ZNK4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE21getSectionStringTableENS_8ArrayRefINS0_13Elf_Shdr_ImplIS5_EEEENS_12function_refIFNS_5ErrorERKNS_5TwineEEEE: error: relocation refers to local symbol "" [749], which is defined in a discarded section
  section group signature: "_ZNK4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE21getSectionStringTableENS_8ArrayRefINS0_13Elf_Shdr_ImplIS5_EEEENS_12function_refIFNS_5ErrorERKNS_5TwineEEEE"
  prevailing definition is from lib/libLLVMRuntimeDyld.a(RuntimeDyldELF.cpp.o)
lib/libLLVMObject.a(ELF.cpp.o):ELF.cpp:__profd__ZNK4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE15getSectionIndexERKNS0_12Elf_Sym_ImplIS5_EENS_8ArrayRefIS8_EENS0_10DataRegionINS3_6detail31packed_endian_specific_integralIjLS4_1ELm1ELm1EEEEE: error: relocation refers to local symbol "" [753], which is defined in a discarded section
  section group signature: "_ZNK4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE15getSectionIndexERKNS0_12Elf_Sym_ImplIS5_EENS_8ArrayRefIS8_EENS0_10DataRegionINS3_6detail31packed_endian_specific_integralIjLS4_1ELm1ELm1EEEEE"
  prevailing definition is from lib/libLLVMRuntimeDyld.a(RuntimeDyldELF.cpp.o)

etc.

This is broken with lld too:

ld.lld: error: relocation refers to a discarded section: .text._ZNK4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE19getRelocationSymbolERKNS0_12Elf_Rel_ImplIS5_Lb0EEEPKNS0_13Elf_Shdr_ImplIS5_EE
>>> defined in lib/libLLVMJITLink.a(ELF_i386.cpp.o)
>>> section group signature: _ZNK4llvm6object7ELFFileINS0_7ELFTypeILNS_7support10endiannessE1ELb0EEEE19getRelocationSymbolERKNS0_12Elf_Rel_ImplIS5_Lb0EEEPKNS0_13Elf_Shdr_ImplIS5_EE
>>> prevailing definition is in lib/libLLVMObject.a(ELF.cpp.o)
>>> referenced by ELF_i386.cpp
>>>               ELF_i386.cpp.o:(__llvm_prf_data+0x18) in archive lib/libLLVMJITLink.a

Thanks for the report. I'll revert and try to reland once it's working more reliably.

paulkirth reopened this revision.Nov 15 2022, 7:40 PM

@glandium do you have a reproducer? or is there a public corpus I can build clang w/ to reproduce?

This revision is now accepted and ready to land.Nov 15 2022, 7:40 PM

Try building clang with -DLLVM_BUILD_INSTRUMENTED=IR with a clang that contains your patch. If that's not enough, try adding -DLLVM_LINK_LLVM_DYLIB=ON. If that's not enough, I'll try to find the right set of flags from what we're using here.

Thank you that’s super helpful.

gulfem added a subscriber: gulfem.Nov 16 2022, 12:15 PM
paulkirth updated this revision to Diff 479469.Dec 1 2022, 4:27 PM

Add additional test

I'm stil investigating some issues around the references to dropped sections,
but was able to reduce at least one case down to something managable. I've
placed it into the runtime tests, since it requires a linker to detect.

Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2022, 4:27 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
paulkirth updated this revision to Diff 479473.Dec 1 2022, 4:37 PM
paulkirth retitled this revision from [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)

Improve test discription and summary

paulkirth updated this revision to Diff 479763.Dec 2 2022, 3:36 PM

Move the logic for getting pointers into a helper and change update the alias's linkage and visibility if it is in a COMDAT.

paulkirth updated this revision to Diff 479790.Dec 2 2022, 6:48 PM

Avoid complexity with COMDAT, since we don't get any benefit in those cases. We
actually end up generating an alias that always is the same linkage and
visibility as the original function, so just use the public symbol, which is
more correct.

paulkirth updated this revision to Diff 480279.Dec 5 2022, 4:30 PM

Restore using hidden alias with COMDAT functions. It does impact the number of relocations, and is a better semantic match. It also eliminates an entry in the dynamic symbol table.

  • Add compile tests for the comdat and non-comdat cases
  • add an end-to-end test in compiler-rt's profile tests
paulkirth added inline comments.Dec 5 2022, 4:38 PM
compiler-rt/test/profile/instrprof-discarded-comdat.ll
1 ↗(On Diff #480279)

This test is a bit redundant with the one above. I had some trouble updating the CMake dependencies of the check-profile target to include opt and llc, so I included the c++ test.

I'm happy to remove either one, but I'm not certain which one is preferable. If we keep this version, we also need to update the CMake files to reflect the new dependency. I don't think there are any changes required for the C++ test.

paulkirth added inline comments.Dec 5 2022, 4:50 PM
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
826

@xur, I think I've worked through a solution that will work here, and solve the issue for Fuchsia that we discussed.

Does this look correct to you? The logic is now: if we need the address for profd, get a private alias when we can. If the function is a comdat, only get a hidden one, if possible.

Do you foresee any issues with this approach?

abrachet added inline comments.
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
837
849

I'm not sure what the linkage should be here, but it doesn't feel right that it should correspond with the functions linkage. At least for ELF, {LinkOnce,Weak}{Any,ODR}Linkage (I think) will all be .weak so this will be fine, but presumably there's more nuance on COFF. Maybe something to investigate.

850

If it hasLocalLinkage() we can just use it's name directly like in the if (Fn->isDeclarationForLinker()) above and not worry about creating an alias at all.

I think it's also the case that Fn->hasComdat() and Fn->hasLocalLinkage() cannot be mutually true per your comment above.

paulkirth added inline comments.Dec 6 2022, 9:55 AM
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
837

Good suggestion. Thank you.

849

I'm not exactly sure either, but using the same linkage seems acceptable, since it should be no worse than using the function. Its also how we generate aliases for relative vtables (https://github.com/llvm/llvm-project/blob/a602f76a2406cc3edd6b297ede3583b26513a34c/clang/lib/CodeGen/CGVTables.cpp#L982). There we use the existing linkage if the function is comdat. The case for vtables may have a more limited range of possibilities though, so maybe the same approach isn't warranted.

As you mentioned, I'm also not sure what the approach should be for COFF, since it seems there are a number of places where we handle it specially.

Are you aware of any issues that could arise if we use the Function's linkage?

850

I'm almost 100% certain this case cannot happen based on the check in ShouldRecordFunctionAddress(), but I wanted to keep the logic here clear, and not dependent on the internals of that check.

But maybe it makes more sense to move the hasLocalLinkage() check up, as you suggested, and simplify this case. I'll test that, and if it works, I can update the patch accordingly.

leonardchan added inline comments.
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
826

I could be wrong, but are there any flaws with simplifying this function to:

if (!shouldRecordFunctionAddr(Fn) || Fn->isDeclarationForLinker())
  return ConstantPointerNull::get(Int8PtrTy);

if (Fn->isDSOLocal() || Fn->isImplicitDSOLocal())
  return Fn;

auto *GA = GlobalAlias::create(Fn->getLinkage(), Fn->getName(), Fn);
GA->setVisibility(GlobalValue::VisibilityTypes::HiddenVisibility);
return GA;

?

I think this would have the following desired results for these interesting cases:

  • Fn is hidden/dso-local regardless of linkage or comdat-ness -> Fn is used resulting in RELATIVE reloc
    • This also covers local linkage symbols, hidden weak symbols, and hidden global symbols regardless of comdat
  • Fn is global linkage, default visibility, no comdat -> Alias with hidden visibility and global linkage used, resulting in RELATIVE reloc
  • Fn is weak linkage, default visibility, no comdat -> Alias with hidden visibility and weak linkage used, resulting in RELATIVE reloc
  • Fn is weak linkage, default visibility, in comdat -> Alias with hidden visibility and weak linkage used, resulting in RELATIVE reloc, *but* the final value in __llvm_prf_data could be undefined, which I think is ok here still

I don't *think* there should ever be a case where we have a symbol with global linkage, default visibility, and in a comdat.

paulkirth added inline comments.Dec 6 2022, 2:34 PM
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
826

The only time we want to insert nullptr is when we should record no address at all. The case where we can’t emit an alias should still use the function pointer.

I’m also not sure how that is any simpler than the change suggested above. I haven’t uploaded that yet, since I’m still testing it on some larger code bases. But so far it seems to work just as well.

Also as mentioned we need to make sure this still works for COFF. I’m not sure what you’ve suggested is equivalent.

paulkirth updated this revision to Diff 480638.Dec 6 2022, 2:49 PM

Move check for hasLocalLinkage() into the check for Fn->isDelcarationForLinker()

  • update comments
  • fix typos
  • add check to test
paulkirth marked an inline comment as done.Dec 6 2022, 2:50 PM
paulkirth added inline comments.Dec 6 2022, 3:03 PM
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
826

@leonardchan Do you think this is a bit closer to what you were thinking.

Also, FYI, the bitcasting seems to be required, otherwise we run into assertion failures later.

leonardchan added inline comments.Dec 6 2022, 3:43 PM
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
826

Yeah it looks mostly similar. The only differences I can see are that aliases could be made even if the original Fn is hidden. But having an alias emitted in the way described now I think shouldn't break anything. Feel free to take the Fn->isDSOLocal() || Fn->isImplicitDSOLocal() check as just a suggestion though. My line of thinking is if we do want to create an alias, then we only do so if we absolutely need to.

paulkirth added inline comments.Dec 6 2022, 3:46 PM
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
826

Well, whenever we can, I think we try to use the private alias. I think that is a better mapping of intent here, but maybe it all boils down to the same thing.

paulkirth updated this revision to Diff 481094.Dec 7 2022, 3:56 PM

Remove redundant test case from compiler-rt profile tests

The IR test required opt and llc, but the CMake dependencies aren't set
up for those tools. The C++ test case covers this situation equally well, and
is no more complicated, but won't require any build changes.

paulkirth updated this revision to Diff 481098.Dec 7 2022, 4:19 PM

Rebase. Not sure why the last merge failed though ...

paulkirth updated this revision to Diff 481105.Dec 7 2022, 4:26 PM
paulkirth edited the summary of this revision. (Show Details)

Update summary

This revision was landed with ongoing or failed builds.Dec 8 2022, 9:37 AM
This revision was automatically updated to reflect the committed changes.
paulkirth reopened this revision.Dec 8 2022, 10:16 AM
This revision is now accepted and ready to land.Dec 8 2022, 10:16 AM
paulkirth updated this revision to Diff 481362.Dec 8 2022, 10:44 AM

Disable windows test, since -fPIC isn't supported with clang.exe

paulkirth updated this revision to Diff 481365.Dec 8 2022, 10:54 AM
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)

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.Jan 9 2023, 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.Jan 9 2023, 3:36 PM

Rebase correctly

This revision was landed with ongoing or failed builds.Jan 11 2023, 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.Jan 17 2023, 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.Jan 17 2023, 3:52 PM
paulkirth updated this revision to Diff 490309.Jan 18 2023, 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.Jan 19 2023, 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.Jan 19 2023, 12:54 PM
paulkirth updated this revision to Diff 491928.Jan 24 2023, 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.