This is an archive of the discontinued LLVM Phabricator instance.

[InstrProfiling] Put instrumentation into comdat group with function
AbandonedPublic

Authored by phosek on Jul 1 2019, 3:58 PM.

Details

Reviewers
davidxl
vsk
Summary

Currently, per-function instrumentation data don't use section groups
except for functions that already are in one and symbols with external
linkage. Consequentially, profiling instrumentation doesn't support
--gc-sections: if the function symbol is discarded, linker doesn't know
what other (instrumentation) data needs to be discarded as well. This
breaks any code that relies on --gc-sections semantics, and even for
other code that doesn't can lead to size increase.

This change moves per-function instrumentation data into its own section
group together with the function itself. This allows all instrumentation
to "travel together" with the symbol. If linker decides to discard the
symbol, it'll discard the entire group including all sections with the
instrumentation. The same strategy is already used by sancov.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.Jul 1 2019, 3:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2019, 3:58 PM
davidxl added inline comments.Jul 1 2019, 4:28 PM
llvm/lib/ProfileData/InstrProf.cpp
1055

See the comments in the code deleted by this patch.

Here you reverse the logic. IIRC, it will greatly increase the size of the profile data as the counter and per function data for externally available symbols won't be commoned.

phosek updated this revision to Diff 207675.Jul 2 2019, 5:46 PM
phosek marked 2 inline comments as done.
phosek added inline comments.
llvm/lib/ProfileData/InstrProf.cpp
1055

I've reverted that part of the change and checked on an example that those symbols are now being commoned.

phosek updated this revision to Diff 207682.Jul 2 2019, 5:57 PM

Do you have a functional test case to show what this patch is trying to accomplish/fix?

phosek added a comment.Jul 9 2019, 7:46 PM

Do you have a functional test case to show what this patch is trying to accomplish/fix?

What kind of test do you have in mind? You mean and E2E test in LLVM or a real-world use case? The only issue with E2E test is that it's going to require linker to verify that we can indeed GC the unused symbols together with the relevant part of the instrumentation, is that OK?

Some end-to-end tests as in compiler-rt are probably helpful here.

pcc added a subscriber: pcc.Dec 2 2019, 4:41 PM

Should this be using !associated metadata on ELF instead of comdats?

In D64045#1766212, @pcc wrote:

Should this be using !associated metadata on ELF instead of comdats?

I agree that !associated metadata is a better fit than comdats, the problem is that !associated metadata is lowered to SHF_LINK_ORDER which is not the correct representation; the intended use case for SHF_LINK_ORDER is to control the section output order (see https://groups.google.com/g/generic-abi/c/avQCrdIALKE) which is not the case here. I think the most appropriate representation in ELF is section group (just a plain section group, not comdat), but there's no way to currently express that in LLVM IR. One option would be to change the lowering of !associated metadata to use section groups and then use !associated metadata in this change.

phosek abandoned this revision.Jun 8 2020, 1:36 PM

Superseded by D76802.