Page MenuHomePhabricator

[profile] Do not cache __llvm_profile_get_filename result
ClosedPublic

Authored by vsk on Oct 17 2019, 1:56 PM.

Details

Summary

When the %m filename pattern is used, the filename is unique to each
image, so the cached value is wrong.

It struck me that the full filename isn't something that's recomputed
often, so perhaps it doesn't need to be cached at all.

@davidxl pointed out we can go further and just hide lprofCurFilename.

rdar://55137071

Diff Detail

Event Timeline

vsk created this revision.Oct 17 2019, 1:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2019, 1:56 PM

shared lib should have different profile name from the main executable so the test should check that two file names are not identical?

vsk updated this revision to Diff 225535.Oct 17 2019, 3:00 PM

The test was not written clearly. I've added a clarifying comment. The test should exit with code 1 if the two filenames compare equal.

I see.

Even without the patch, I saw two different files are created with %m. How is the bug triggered?

vsk added a comment.Oct 17 2019, 4:20 PM

I see.

Even without the patch, I saw two different files are created with %m. How is the bug triggered?

An internal adopter is making use of the libprofile APIs to dump profile data when a message is received in a server. I'm not sure what the exact API usage looks like (why not just call __llvm_profile_dump directly?). Perhaps the filename is used for logging.

maybe we should make lprofCurFilename hidden?

vsk added a comment.Oct 17 2019, 4:47 PM

maybe we should make lprofCurFilename hidden?

Oh, you mean to stop relying on the dynamic loader coalescing all copies of the symbol? I think that would be nice.

Currently the runtime only relies on a coalesced lprofCurFilename symbol to avoid truncating a raw profile twice (IIRC, if FilenamePat is set => do not truncate). But we could easily use getenv/setenv to accomplish the same thing. This might simplify the code, the only cost is parsing the filename repeatedly.

If we can clean up the code, that will be great.

vsk updated this revision to Diff 225712.Oct 18 2019, 4:05 PM
vsk edited the summary of this revision. (Show Details)

Hide lprofCurFilename, per David's suggestion. This requires deleting 'instrprof-set-filename-shared.test'.

davidxl accepted this revision.Oct 18 2019, 4:22 PM

Thanks. lgtm

This revision is now accepted and ready to land.Oct 18 2019, 4:22 PM
vsk added a comment.Oct 18 2019, 4:24 PM

FTR, I'm a bit conflicted about the latest version of this patch. On one hand, hiding lprofCurFilename decouples all of the copies of the profiling runtime in a process. On the other, it may cause a regression for users who use the set-filename API with instrumented DSOs. A quick search shows that we have no internal adopters who rely on the behavior, so I'm inclined to go ahead with this.

This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 18 2019, 4:32 PM
Herald added subscribers: Restricted Project, cfe-commits. · View Herald Transcript