Reland [pgo] Avoid introducing relocations by using private alias

[paulkirth]

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.

[paulkirth]

Fix formatting

[phosek]

llvm/test/Transforms/PGOProfile/prof_avoid_relocs.ll

27

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?

[paulkirth]

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]

llvm/test/Transforms/PGOProfile/prof_avoid_relocs.ll

27

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]

LGTM

[glandium]

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.

[glandium]

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

[paulkirth]

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

[paulkirth]

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

[glandium]

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.

[paulkirth]

Thank you that’s super helpful.

[paulkirth]

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.

[paulkirth]

Improve test discription and summary

[paulkirth]

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]

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]

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]

Fix typo

[paulkirth]

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]

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]

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]

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]

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]

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]

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

• update comments
• fix typos
• add check to test

[paulkirth]

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]

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]

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]

Rebase

[paulkirth]

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]

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

[paulkirth]

Update summary

[paulkirth]

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

[paulkirth]

Update summary

[steven_wu]

This still breaks Darwin bots: https://green.lab.llvm.org/green/job/clang-stage1-RA/32237/

[paulkirth]

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

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

[paulkirth]

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

[glandium]

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

[paulkirth]

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

[glandium]

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]

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]

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

rebase

[paulkirth]

Rebase

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

[hans]

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]

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

[paulkirth]

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

Rebase.

• move test from linux only to also run on mac
• update summary

[paulkirth]

Rebase correctly

[paulkirth]

Rebase

[aeubanks]

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

[paulkirth]

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

In D137982#4059724, @paulkirth wrote:

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

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

[paulkirth]

Rebase.

• Avoid introducing alias at all in the comdat + hidden visibility case
• simplify test case as in https://reviews.llvm.org/D141727

[aeubanks]

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

[paulkirth]

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

[paulkirth]

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]

Rebase and update summary.

compiler-rt/test/profile/Linux/instrprof-discarded-comdat.cpp

+// Check that instrprof does not introduce references to discarded sections when
+// using comdats.
+//
+// Occasionally, it is possible that the same function can be compiled in
+// different TUs with slightly different linkages, e.g., due to different
+// compiler options. However, if these are comdat functions, a single
+// implementation will be chosen at link time. we want to ensure that the
+// profiling data does not contain a reference to the discarded section.
+
+// REQUIRES: linux
+// RUN: mkdir -p %t.d
+// RUN: %clangxx_pgogen -O2 -fPIC -ffunction-sections -fdata-sections -c %s -o %t.d/a1.o -DOBJECT_1 -mllvm -disable-preinline
+// RUN: %clangxx_pgogen -O2 -fPIC -ffunction-sections -fdata-sections -c %s -o %t.d/a2.o
+// RUN: %clangxx -fPIC -shared -o %t.d/liba.so %t.d/a1.o %t.d/a2.o 2>&1 | FileCheck %s --allow-empty
+
+// Ensure that we don't get an error when linking
+// CHECK-NOT: relocation refers to a discarded section: .text._ZN1CIiE1fEi
+
+template <typename T> struct C {
+  void f(T x);
+  int g(T x) {
+    f(x);
+    return v;
+  }
+  int v;
+};
+
+template <typename T>
+#ifdef OBJECT_1
+__attribute__((weak))
+#else
+__attribute__((noinline))
+#endif
+void C<T>::f(T x) {
+  v += x;
+}
+
+#ifdef OBJECT_1
+int foo() {
+  C<int> c;
+  c.f(1);
+  return c.g(2);
+}
+#else
+int bar() {
+  C<int> c;
+  c.f(3);
+  return c.g(4);
+}
+#endif

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

static inline bool shouldRecordFunctionAddr(Function *F) {
   // exists, the vtable will only be emitted in the TU where the key method
   // is defined. In a TU where vtable is not available, the function won't
   // be 'addresstaken'. If its address is not recorded here, the profile data
   // with missing address may be picked by the linker leading  to missing
   // indirect call target info.
   return F->hasAddressTaken() || F->hasLinkOnceLinkage();
 }
 
+static inline Constant *getFuncAddrForProfData(Function *Fn) {
+  auto *Int8PtrTy = Type::getInt8PtrTy(Fn->getContext());
+  // Store a nullptr in __llvm_profd, if we shouldn't use a real address
+  if (!shouldRecordFunctionAddr(Fn))
+    return ConstantPointerNull::get(Int8PtrTy);
+
+  // If we can't use an alias, we must use the public symbol, even though this
+  // may require a symbolic relocation. When the function has local linkage, we
+  // can use the symbol directly without introducing relocations.
+  if (Fn->isDeclarationForLinker() || Fn->hasLocalLinkage())
+    return ConstantExpr::getBitCast(Fn, Int8PtrTy);
+
+  // When possible use a private alias to avoid symbolic relocations.
+  auto *GA = GlobalAlias::create(GlobalValue::LinkageTypes::PrivateLinkage,
+                                 Fn->getName(), Fn);
+
+  // When the instrumented function is a COMDAT function, we cannot use a
+  // private alias. If we did, we would create reference to a local label in
+  // this function's section. If this version of the function isn't selected by
+  // the linker, then the metadata would introduce a reference to a discarded
+  // section. So, for COMDAT functions, we need to adjust the linkage of the
+  // alias. Using hidden visibility avoids a dynamic relocation and an entry in
+  // the dynamic symbol table.
+  if (Fn->hasComdat()) {
+    GA->setLinkage(Fn->getLinkage());
+    GA->setVisibility(GlobalValue::VisibilityTypes::HiddenVisibility);
+  }
+  return ConstantExpr::getBitCast(GA, Int8PtrTy);
+}
+
 static bool needsRuntimeRegistrationOfSectionRange(const Triple &TT) {
   // Don't do this for Darwin.  compiler-rt uses linker magic.
   if (TT.isOSDarwin())
     return false;
   // Use linker script magic to get data/cnts/name start/end.
   if (TT.isOSAIX() || TT.isOSLinux() || TT.isOSFreeBSD() || TT.isOSNetBSD() ||
       TT.isOSSolaris() || TT.isOSFuchsia() || TT.isPS() || TT.isOSWindows())
     return false;
InstrProfiling::getOrCreateRegionCounters(InstrProfInstBase *Inc) {
   auto *Int16Ty = Type::getInt16Ty(Ctx);
   auto *Int16ArrayTy = ArrayType::get(Int16Ty, IPVK_Last + 1);
   Type *DataTypes[] = {
 #define INSTR_PROF_DATA(Type, LLVMType, Name, Init) LLVMType,
 #include "llvm/ProfileData/InstrProfData.inc"
   };
   auto *DataTy = StructType::get(Ctx, makeArrayRef(DataTypes));
 
-  Constant *FunctionAddr = shouldRecordFunctionAddr(Fn)
-                               ? ConstantExpr::getBitCast(Fn, Int8PtrTy)
-                               : ConstantPointerNull::get(Int8PtrTy);
+  Constant *FunctionAddr = getFuncAddrForProfData(Fn);
 
   Constant *Int16ArrayVals[IPVK_Last + 1];
   for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
     Int16ArrayVals[Kind] = ConstantInt::get(Int16Ty, PD.NumValueSites[Kind]);
 
   // If the data variable is not referenced by code (if we don't emit
   // @llvm.instrprof.value.profile, NS will be 0), and the counter keeps the
   // data variable live under linker GC, the data variable can be private. This

llvm/test/Transforms/PGOProfile/comdat.ll

 ;; Test comdat functions.
 ; RUN: opt < %s -mtriple=x86_64-linux -passes=pgo-instr-gen,instrprof -S | FileCheck %s --check-prefixes=ELF
 ; RUN: opt < %s -mtriple=x86_64-windows -passes=pgo-instr-gen,instrprof -S | FileCheck %s --check-prefixes=COFF
 
 $linkonceodr = comdat any
 $weakodr = comdat any
+$weak = comdat any
+$linkonce = comdat any
 
 ;; profc/profd have hash suffixes. This definition doesn't have value profiling,
 ;; so definitions with the same name in other modules must have the same CFG and
 ;; cannot have value profiling, either. profd can be made private for ELF.
 ; ELF:   @__profc_linkonceodr.[[#]] = linkonce_odr hidden global {{.*}} comdat, align 8
 ; ELF:   @__profd_linkonceodr.[[#]] = private global {{.*}} comdat($__profc_linkonceodr.[[#]]), align 8
 ; COFF:  @__profc_linkonceodr.[[#]] = linkonce_odr hidden global {{.*}} comdat, align 8
 ; COFF:  @__profd_linkonceodr.[[#]] = linkonce_odr hidden global {{.*}} comdat, align 8
 define linkonce_odr void @linkonceodr() comdat {
   ret void
 }
 
 ;; weakodr in a comdat is not renamed. There is no guarantee that definitions in
 ;; other modules don't have value profiling. profd should be conservatively
 ;; non-private to prevent a caller from referencing a non-prevailing profd,
 ;; causing a linker error.
 ; ELF:   @__profc_weakodr = weak_odr hidden global {{.*}} comdat, align 8
 ; ELF:   @__profd_weakodr = weak_odr hidden global {{.*}} comdat($__profc_weakodr), align 8
 ; COFF:  @__profc_weakodr = weak_odr hidden global {{.*}} comdat, align 8
 ; COFF:  @__profd_weakodr = weak_odr hidden global {{.*}} comdat, align 8
 define weak_odr void @weakodr() comdat {
   ret void
 }
+
+;; weak in a comdat is not renamed. There is no guarantee that definitions in
+;; other modules don't have value profiling. profd should be conservatively
+;; non-private to prevent a caller from referencing a non-prevailing profd,
+;; causing a linker error.
+; ELF:   @__profc_weak = weak hidden global {{.*}} comdat, align 8
+; ELF:   @__profd_weak = weak hidden global {{.*}} comdat($__profc_weak), align 8
+; COFF:  @__profc_weak = weak hidden global {{.*}} comdat, align 8
+; COFF:  @__profd_weak = weak hidden global {{.*}} comdat, align 8
+define weak void @weak() comdat {
+  ret void
+}
+
+;; profc/profd have hash suffixes. This definition doesn't have value profiling,
+;; so definitions with the same name in other modules must have the same CFG and
+;; cannot have value profiling, either. profd can be made private for ELF.
+; ELF:   @__profc_linkonce.[[#]] = linkonce hidden global {{.*}} comdat, align 8
+; ELF:   @__profd_linkonce.[[#]] = private global {{.*}} comdat($__profc_linkonce.[[#]]), align 8
+; COFF:  @__profc_linkonce.[[#]] = linkonce hidden global {{.*}} comdat, align 8
+; COFF:  @__profd_linkonce.[[#]] = linkonce hidden global {{.*}} comdat, align 8
+define linkonce void @linkonce() comdat {
+  ret void
+}
+
+; Check that comdat aliases are hidden for all linkage types
+; ELF:   @linkonceodr.1 = linkonce_odr hidden alias void (), ptr @linkonceodr
+; ELF:   @weakodr.2 = weak_odr hidden alias void (), ptr @weakodr
+; ELF:   @weak.3 = weak hidden alias void (), ptr @weak
+; ELF:   @linkonce.4 = linkonce hidden alias void (), ptr @linkonce

llvm/test/Transforms/PGOProfile/prof_avoid_relocs.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals
+; RUN: opt -S -passes=pgo-instr-gen,instrprof < %s | FileCheck %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+;; Test that we use private aliases to reference function addresses inside profile data
+; CHECK: @__profd_foo = private global {{.*}}, ptr @foo.1,
+; CHECK-NOT: @__profd_foo = private global {{.*}}, ptr @foo,
+; CHECK: @[[__PROFC_WEAK:[a-zA-Z0-9_$"\\.-]+]] = weak hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat, align 8
+; CHECK: @[[__PROFD_WEAK:[a-zA-Z0-9_$"\\.-]+]] = private global { i64, i64, i64, ptr, ptr, i32, [2 x i16] } { i64 -5028622335731970946, i64 742261418966908927, i64 sub (i64 ptrtoint (ptr @__profc_weak to i64), i64 ptrtoint (ptr @__profd_weak to i64)), ptr @weak.2, ptr null, i32 1, [2 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profc_weak), align 8
+; CHECK: @[[__PROFC_LINKONCE:[a-zA-Z0-9_$"\\.-]+]] = linkonce hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat, align 8
+; CHECK: @[[__PROFD_LINKONCE:[a-zA-Z0-9_$"\\.-]+]] = private global { i64, i64, i64, ptr, ptr, i32, [2 x i16] } { i64 -121947654961992603, i64 742261418966908927, i64 sub (i64 ptrtoint (ptr @__profc_linkonce to i64), i64 ptrtoint (ptr @__profd_linkonce to i64)), ptr @linkonce.3, ptr null, i32 1, [2 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profc_linkonce), align 8
+; CHECK: @[[__PROFC_WEAKODR:[a-zA-Z0-9_$"\\.-]+]] = weak_odr hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat, align 8
+; CHECK: @[[__PROFD_WEAKODR:[a-zA-Z0-9_$"\\.-]+]] = private global { i64, i64, i64, ptr, ptr, i32, [2 x i16] } { i64 -4807837289933096997, i64 742261418966908927, i64 sub (i64 ptrtoint (ptr @__profc_weakodr to i64), i64 ptrtoint (ptr @__profd_weakodr to i64)), ptr @weakodr.4, ptr null, i32 1, [2 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profc_weakodr), align 8
+; CHECK: @[[__PROFC_LINKONCEODR:[a-zA-Z0-9_$"\\.-]+]] = linkonce_odr hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat, align 8
+; CHECK: @[[__PROFD_LINKONCEODR:[a-zA-Z0-9_$"\\.-]+]] = private global { i64, i64, i64, ptr, ptr, i32, [2 x i16] } { i64 4214081367395809689, i64 742261418966908927, i64 sub (i64 ptrtoint (ptr @__profc_linkonceodr to i64), i64 ptrtoint (ptr @__profd_linkonceodr to i64)), ptr @linkonceodr.5, ptr null, i32 1, [2 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profc_linkonceodr), align 8
+; CHECK: @[[__PROFC_AVAILABLE_EXTERNALLY_742261418966908927:[a-zA-Z0-9_$"\\.-]+]] = linkonce_odr hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat, align 8
+; CHECK: @[[__PROFD_AVAILABLE_EXTERNALLY_742261418966908927:[a-zA-Z0-9_$"\\.-]+]] = private global { i64, i64, i64, ptr, ptr, i32, [2 x i16] } { i64 -8510055422695886042, i64 742261418966908927, i64 sub (i64 ptrtoint (ptr @__profc_available_externally.742261418966908927 to i64), i64 ptrtoint (ptr @__profd_available_externally.742261418966908927 to i64)), ptr null, ptr null, i32 1, [2 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profc_available_externally.742261418966908927), align 8
+
+;; Ensure when not instrumenting a non-comdat function, then if we generate an
+;; alias, then it is private. We check comdat versions in comdat.ll
+; CHECK: @foo.1 = private alias i32 (i32), ptr @foo
+; CHECK: @[[WEAK_2:[a-zA-Z0-9_$"\\.-]+]] = private alias void (), ptr @weak
+; CHECK: @[[LINKONCE_3:[a-zA-Z0-9_$"\\.-]+]] = private alias void (), ptr @linkonce
+; CHECK: @[[WEAKODR_4:[a-zA-Z0-9_$"\\.-]+]] = private alias void (), ptr @weakodr
+; CHECK: @[[LINKONCEODR_5:[a-zA-Z0-9_$"\\.-]+]] = private alias void (), ptr @linkonceodr
+
+;; We should never generate an alias for available_externally functions
+; CHECK-NOT: @[[AVAILABLE_EXTERNALLY_6:[a-zA-Z0-9_$"\\.-]+]] = private alias void (), ptr @available_externally
+
+define i32 @foo(i32 %0) {
+; CHECK-LABEL: @foo(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[PGOCOUNT:%.*]] = load i64, ptr @__profc_foo, align 8
+; CHECK-NEXT:    [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1
+; CHECK-NEXT:    store i64 [[TMP1]], ptr @__profc_foo, align 8
+; CHECK-NEXT:    ret i32 0
+entry:
+  ret i32 0
+}
+
+define weak void @weak() {
+; CHECK-LABEL: @weak(
+; CHECK-NEXT:    [[PGOCOUNT:%.*]] = load i64, ptr @__profc_weak, align 8
+; CHECK-NEXT:    [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1
+; CHECK-NEXT:    store i64 [[TMP1]], ptr @__profc_weak, align 8
+; CHECK-NEXT:    ret void
+  ret void
+}
+
+define linkonce void @linkonce() {
+; CHECK-LABEL: @linkonce(
+; CHECK-NEXT:    [[PGOCOUNT:%.*]] = load i64, ptr @__profc_linkonce, align 8
+; CHECK-NEXT:    [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1
+; CHECK-NEXT:    store i64 [[TMP1]], ptr @__profc_linkonce, align 8
+; CHECK-NEXT:    ret void
+  ret void
+}
+
+define weak_odr void @weakodr() {
+; CHECK-LABEL: @weakodr(
+; CHECK-NEXT:    [[PGOCOUNT:%.*]] = load i64, ptr @__profc_weakodr, align 8
+; CHECK-NEXT:    [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1
+; CHECK-NEXT:    store i64 [[TMP1]], ptr @__profc_weakodr, align 8
+; CHECK-NEXT:    ret void
+  ret void
+}
+
+define linkonce_odr void @linkonceodr() {
+; CHECK-LABEL: @linkonceodr(
+; CHECK-NEXT:    [[PGOCOUNT:%.*]] = load i64, ptr @__profc_linkonceodr, align 8
+; CHECK-NEXT:    [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1
+; CHECK-NEXT:    store i64 [[TMP1]], ptr @__profc_linkonceodr, align 8
+; CHECK-NEXT:    ret void
+  ret void
+}
+
+define available_externally void @available_externally(){
+; CHECK-LABEL: @available_externally(
+; CHECK-NEXT:    [[PGOCOUNT:%.*]] = load i64, ptr @__profc_available_externally.742261418966908927, align 8
+; CHECK-NEXT:    [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1
+; CHECK-NEXT:    store i64 [[TMP1]], ptr @__profc_available_externally.742261418966908927, align 8
+; CHECK-NEXT:    ret void
+  ret void
+}