Page MenuHomePhabricator

[InstrProfiling] Put instrumentation into comdat group with function
Needs ReviewPublic

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

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
1054

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
1054

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.