Page MenuHomePhabricator

[MemProf] Allow the binary to specify the profile output filename
ClosedPublic

Authored by tejohnson on Oct 8 2020, 5:52 PM.

Details

Summary

This will allow the output directory to be specified by a build time
option, similar to the directory specified for regular PGO profiles via
-fprofile-generate=. The memory profiling instrumentation pass will
set up the variable. This is the same mechanism used by the PGO
instrumentation and runtime.

Depends on D87120 and D89629.

Diff Detail

Event Timeline

tejohnson created this revision.Oct 8 2020, 5:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2020, 5:52 PM
Herald added subscribers: Restricted Project, wenlei. · View Herald Transcript
tejohnson requested review of this revision.Oct 8 2020, 5:52 PM

Profile runtime has 4 ways of setting output: 1) default 2) compiler command line arg 3) environment variable at runtime; and 4) user invocation of runtime API at runtime. The order of the precedence is 4> 3 > 2 > 1. Is the behavior here consistent with that?

LGTM but I have no opinion on David's request.

compiler-rt/lib/memprof/memprof_rtl.cpp
31

not needed?

compiler-rt/lib/sanitizer_common/sanitizer_file.cpp
71–92 ↗(On Diff #297091)

looks correct, but it would be nice to have sanitizer_common as a separate patch

compiler-rt/test/memprof/TestCases/atexit_stats.cpp
4 ↗(On Diff #297091)

It's not clear why this is needed before reading patch description.
Seems unrelated to __memprof_profile_filename and can be a separate patch.

Profile runtime has 4 ways of setting output: 1) default 2) compiler command line arg 3) environment variable at runtime; and 4) user invocation of runtime API at runtime. The order of the precedence is 4> 3 > 2 > 1. Is the behavior here consistent with that?

The runtime I recently upstreamed has #3, and this patch adds #1 (well there is a default in the already upstreamed stuff, but it is stderr) and #2. A follow on patch will add #4. The order of precedence will be the same as in the profile runtime (it is already 3>2>1).

compiler-rt/lib/memprof/memprof_rtl.cpp
31

This is to ensure that the if below at line 182 will fail when the compiler has not set up a strong version of this variable.

compiler-rt/lib/sanitizer_common/sanitizer_file.cpp
71–92 ↗(On Diff #297091)

Ok, I can split this out.

compiler-rt/test/memprof/TestCases/atexit_stats.cpp
4 ↗(On Diff #297091)

I can just move this to the follow on patch that changes the compiler which affects the defaults, requiring this change.

vitalybuka added inline comments.Oct 16 2020, 4:38 PM
compiler-rt/lib/memprof/memprof_rtl.cpp
31

Sorry, I asked if initialization is needed. "={0}" I'd expect it's already zeroes.

tejohnson added inline comments.Oct 16 2020, 4:45 PM
compiler-rt/lib/memprof/memprof_rtl.cpp
31

You're right. The global should get zero initialized so the init isn't really necessary. I can remove it.

Address comments.

vitalybuka accepted this revision.Oct 20 2020, 1:46 AM
This revision is now accepted and ready to land.Oct 20 2020, 1:46 AM
This revision was landed with ongoing or failed builds.Oct 22 2020, 8:30 AM
This revision was automatically updated to reflect the committed changes.