Page MenuHomePhabricator

[InstrProfiling] Place __llvm_prf_vnodes and __llvm_prf_names in llvm.used on ELF
ClosedPublic

Authored by MaskRay on Feb 28 2021, 5:10 PM.

Details

Summary

__llvm_prf_vnodes and __llvm_prf_names are used by runtime but not
referenced via relocation in the translation unit.

With -z start-stop-gc (LLD 13 (D96914); GNU ld 2.37 https://sourceware.org/bugzilla/show_bug.cgi?id=27451),
the linker does not let __start_/__stop_ references retain their sections.

Place __llvm_prf_vnodes and __llvm_prf_names in llvm.used to make
them retained by the linker.

This patch changes most existing UsedVars cases to CompilerUsedVars
to reflect the ideal state - if the binary format properly supports
section based GC (dead stripping), llvm.compiler.used should be sufficient.

__llvm_prf_vnodes and __llvm_prf_names are switched to UsedVars
since we want them to be unconditionally retained by both compiler and linker.

Behaviors on COFF/Mach-O are not affected.

Diff Detail

Event Timeline

MaskRay created this revision.Feb 28 2021, 5:10 PM
MaskRay requested review of this revision.Feb 28 2021, 5:10 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 28 2021, 5:10 PM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript
davidxl added inline comments.Mar 1 2021, 9:08 AM
compiler-rt/test/profile/Linux/instrprof-value-merge.c
11 ↗(On Diff #327023)

Can you add tests for other linkers with similar options?

MaskRay updated this revision to Diff 327181.Mar 1 2021, 10:42 AM
MaskRay marked an inline comment as done.

Test GNU ld and gold

davidxl added inline comments.Mar 1 2021, 10:50 AM
compiler-rt/test/profile/Linux/instrprof-value-merge.c
1 ↗(On Diff #327181)

Is it better to split the test into multiple RUN file based tests with only one requires linux and lld? Otherwise it seems that adding this will disable the tests on some bots (freebsd or no lld).

MaskRay updated this revision to Diff 327196.Mar 1 2021, 11:05 AM

Add instrprof-value-merge-lld.c with REQUIRES: lld-available

davidxl accepted this revision.Mar 1 2021, 11:18 AM

LGTM (the a.out file is probably accidentally added).

This revision is now accepted and ready to land.Mar 1 2021, 11:18 AM
MaskRay updated this revision to Diff 327244.Mar 1 2021, 12:28 PM

Drop accidental a.out

MaskRay edited the summary of this revision. (Show Details)Mar 1 2021, 1:41 PM
This revision was landed with ongoing or failed builds.Mar 1 2021, 1:43 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Mar 3 2021, 7:21 AM

This breaks instrprof-value-merge.c on our bots (linux only): https://ci.chromium.org/ui/p/chromium/builders/try/linux_upload_clang/1394/overview

It fails in stage 0, which is a pretty vanilla build.

I reverted this for now in 64f5d7e9725b1e1b9d70628e741aa4390795d510

Please let me know if you need help reproducing :)

This breaks instrprof-value-merge.c on our bots (linux only): https://ci.chromium.org/ui/p/chromium/builders/try/linux_upload_clang/1394/overview

It fails in stage 0, which is a pretty vanilla build.

I reverted this for now in 64f5d7e9725b1e1b9d70628e741aa4390795d510

Please let me know if you need help reproducing :)

How can I reproduce it?

This works in normal builds.

MaskRay added a comment.EditedMar 3 2021, 11:03 AM

This breaks instrprof-value-merge.c on our bots (linux only): https://ci.chromium.org/ui/p/chromium/builders/try/linux_upload_clang/1394/overview

It fails in stage 0, which is a pretty vanilla build.

I reverted this for now in 64f5d7e9725b1e1b9d70628e741aa4390795d510

Please let me know if you need help reproducing :)

How can I reproduce it?

This works in normal builds.

@thakis said this is a Ubuntu 14.04 bot. I guess it may have an old binutils where --gc-sections might never work for value profiling properly.
(It reminds me of https://reviews.llvm.org/D97110#2583184 a binutils ~2.28 bug)

I'll omit --gc-sections in instrprof-value-merge.c for now.

MaskRay reopened this revision.Mar 3 2021, 11:30 AM
MaskRay edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Mar 3 2021, 11:31 AM
MaskRay updated this revision to Diff 327867.Mar 3 2021, 11:31 AM

Precommit improved tests

This revision was landed with ongoing or failed builds.Mar 3 2021, 11:32 AM
This revision was automatically updated to reflect the committed changes.