This is an archive of the discontinued LLVM Phabricator instance.

[Profile] Support __llvm_profile_set_file_object in continuous mode.
ClosedPublic

Authored by zequanwu on Aug 17 2021, 2:23 PM.

Details

Summary

Replace D107203, because __llvm_profile_set_file_object is usually used when the
process doesn't have permission to open/create file. That patch trying to copy
from old profile to new profile contradicts with the usage.

Diff Detail

Event Timeline

zequanwu created this revision.Aug 17 2021, 2:23 PM
zequanwu requested review of this revision.Aug 17 2021, 2:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2021, 2:23 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vsk added a comment.Aug 24 2021, 1:45 PM

@zequanwu I apologize for the delay in reviews. I've skimmed this change, and think it's in the right direction. I will try to respond with an in-depth review by the end of this week, if no one beats me to it.

vsk added inline comments.Aug 24 2021, 2:06 PM
compiler-rt/test/profile/ContinuousSyncMode/set-file-object.c
25

Should this also verify that /profdir/<hash>profraw.old exists? IIUC it should, but counter_test() should have 0 coverage.

zequanwu updated this revision to Diff 368504.Aug 24 2021, 5:20 PM
zequanwu marked an inline comment as done.

Check zero count on old profile.

vsk accepted this revision.Aug 27 2021, 11:44 AM

Thanks, lgtm with one concern (see inline).

compiler-rt/lib/profile/InstrProfilingFile.c
1167

You might already be aware of this limitation, but a heads-up just in case: the continuous sync mode does not handle keeping VP data up-to-date. While it's fine to write it out in the ProfileFileSize = 0 case, in general this is not a well-supported path (yet).

This revision is now accepted and ready to land.Aug 27 2021, 11:44 AM
zequanwu updated this revision to Diff 369166.Aug 27 2021, 1:00 PM
zequanwu marked an inline comment as done.

Address comment.

This revision was landed with ongoing or failed builds.Aug 27 2021, 1:09 PM
This revision was automatically updated to reflect the committed changes.

@zequanwu We've been seeing this test fail seemingly randomly on our bots and I think the use of posix_spawn without then waiting for the children to finish might be the cause.

https://lab.llvm.org/buildbot/#/builders/179/builds/942/steps/12/logs/FAIL__Profile-aarch64__set-file-object_c

/home/tcwg-buildslave/worker/clang-aarch64-full-2stage/llvm/compiler-rt/test/profile/ContinuousSyncMode/set-file-object.c:25:11: error: MERGE: expected string not found in input
// MERGE: Function count: 32

And we get:
check:25'0                 X error: no match found
           10:  Function count: 31 
check:25'0     ~~~~~~~~~~~~~~~~~~~~

I think there may be a situation where on a heavily loaded system (we have a large server with many bots) where the last child doesn't write to the file before the llvm-profdata commands run.

I'm trying to reproduce locally to prove it but that's going to be tricky. Would it make sense to wait ("join", I think) on the children anyway to be safe here?

My framework project is hitting this error case in XC13 if (CounterMmap != CountersBegin) {....
Is there any troubleshooting I can perform on my end to resolve this? Code coverage shows at 0% for only one module.