Page MenuHomePhabricator

[profile] Fix file contention causing dropped counts on Windows under -fprofile-generate
ClosedPublic

Authored by Holman on Nov 15 2019, 11:26 AM.

Details

Summary

See PR43425:
https://bugs.llvm.org/show_bug.cgi?id=43425

When writing profile data on Windows we were opening profile file with exclusive read/write access.

In case we are trying to write to the file from multiple processes simultaneously, subsequent calls to CreateFileA would return INVALID_HANDLE_VALUE.

To fix this, I changed to open without exclusive access and then take a lock.

Diff Detail

Event Timeline

Holman created this revision.Nov 15 2019, 11:26 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 15 2019, 11:26 AM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript
Holman edited the summary of this revision. (Show Details)Nov 15 2019, 12:02 PM

Is it possible to add a test?

hans added a comment.Nov 18 2019, 12:31 AM

Please reference PR43425 in the patch description. I don't know if the reproducer from there can be used as a test case, but at least it should be possible to use to check this works manually.

compiler-rt/lib/profile/InstrProfilingUtil.c
222

Does lprofLockFd work on Windows? It looks like it just calls flock() but I didn't know that's a Windows thing?

Holman marked an inline comment as done.Nov 18 2019, 11:22 AM

Please reference PR43425 in the patch description. I don't know if the reproducer from there can be used as a test case, but at least it should be possible to use to check this works manually.

I'm not sure how amenable that is to being a test case, but I did manually verify my fix with it. There actually is a test already but it only runs on Linux. I'm trying to see if I can pull it out to run on Windows as well.

compiler-rt/lib/profile/InstrProfilingUtil.c
222

It's not a Windows thing, but there is a shim for it in compiler-rt/lib/profile/WindowsMMap.c which calls LockFileEx under the hood.

Holman edited the summary of this revision. (Show Details)Nov 18 2019, 11:47 AM
hans accepted this revision.Nov 18 2019, 12:01 PM

lgtm

This revision is now accepted and ready to land.Nov 18 2019, 12:01 PM
hans added a comment.Nov 18 2019, 11:47 PM

Do you have commit access, or would you like me to commit on your behalf?

Holman updated this revision to Diff 230149.Nov 19 2019, 2:17 PM

Added Windows test case.

Do you have commit access, or would you like me to commit on your behalf?

I don't have commit access, so I'd appreciate if you could commit on my behalf.

davidxl accepted this revision.Nov 19 2019, 2:29 PM

thanks for the test case.

hans added a comment.Nov 20 2019, 4:51 AM

Hmm, the test looks more like a unit test for lprofOpenFileEx() rather than something that tests profile merging.

The other tests in profile/ are more like integration tests. Would it be possible to write one for this case also? For example, could we have a test program that spawns N subprocesses, which each call some function, and then we check that the profile data reflects that the function's entry count is N?

Perhaps just add additional checks on the content of the dumped profile to make the test case more complete.

Holman updated this revision to Diff 230307.Nov 20 2019, 12:05 PM

Added test for actual profile merge.

davidxl added inline comments.Nov 20 2019, 12:21 PM
compiler-rt/test/profile/Windows/instrprof-multiwrite.test
6 ↗(On Diff #230307)

add option --counts to check the exact profile counts.

Holman marked an inline comment as done.Nov 20 2019, 12:32 PM
Holman added inline comments.
compiler-rt/test/profile/Windows/instrprof-multiwrite.test
6 ↗(On Diff #230307)

I can add --counts if you want, but the only thing that added is:

Block counts: []

For me, the "Function count: 10" was really the most important output to validate that all calls were recorded.

davidxl added inline comments.Nov 20 2019, 1:25 PM
compiler-rt/test/profile/Windows/instrprof-multiwrite.test
6 ↗(On Diff #230307)

you can add some control flow inside 'foo' to show counters.

Holman updated this revision to Diff 230325.Nov 20 2019, 1:43 PM
Holman set the repository for this revision to rG LLVM Github Monorepo.

Add block counts to test

davidxl accepted this revision.Nov 20 2019, 1:54 PM

lgtm

hans added a comment.Nov 21 2019, 12:53 AM

Thanks! I think instrprof-multiwrite.test is a great test for this.

I'd suggest dropping instrprof-open-file.c and only keep the other one, which is simpler and more targeted towards testing the profile info rather than the function used internally to implement it.

compiler-rt/test/profile/Windows/Inputs/instrprof-multiwrite.c
10 ↗(On Diff #230325)

Since the child_num doesn't really matter for the child process, I'd suggest maybe dropping that to simplify the code.

I suppose an environment variable is still needed to differentiate between child and parent processes, but it could just be set to a constant string.

57 ↗(On Diff #230325)

I know dxf suggested adding some control-flow here, but I don't see why. Isn't just the function entry counts enough to verify that we've merged the profile info successfully? I'd suggest keeping it simple.

65 ↗(On Diff #230325)

This declaration is not needed.

69 ↗(On Diff #230325)

Nit: I'd probably use a #define for the number of children.

compiler-rt/test/profile/Windows/instrprof-multiwrite.test
2 ↗(On Diff #230325)

Maybe "instrprof-multiprocess.c" would be a better name?

6 ↗(On Diff #230325)

Thanks! I think this is a great test.

I'd personally prefer stripping out the expectations below that aren't essential to the test, and only keep the check that foo is entered 10 times, i.e. something like

CHECK: Counters:
CHECK:   foo:
CHECK:     Function count: 10
davidxl added inline comments.Nov 21 2019, 9:22 AM
compiler-rt/test/profile/Windows/Inputs/instrprof-multiwrite.c
57 ↗(On Diff #230325)

Since it is a merging test, it is better IMO to sanity test internal counts are merged properly as well with the simple control flow.

hans added inline comments.Nov 21 2019, 9:58 AM
compiler-rt/test/profile/Windows/Inputs/instrprof-multiwrite.c
57 ↗(On Diff #230325)

But it's not really the merging functionality that's being targeted by this fix and test, it's the ability to write to the same profraw file from multiple processes. The actual merging is supposedly covered by other tests.

But I don't feel very strongly, I just think it would be simpler and make the test more focused to leave this out.

Holman updated this revision to Diff 230507.Nov 21 2019, 12:27 PM
Holman marked 2 inline comments as done.

Change tests per review comments

hans added a comment.Nov 25 2019, 5:19 AM

Sorry for the slow reply; I was out last Friday.

Only one last comment about the %run commands.

compiler-rt/test/profile/Windows/instrprof-multiprocess.test
6

It looks like it's already %run on the line above. Is this second line necessary?

Holman updated this revision to Diff 231093.Nov 26 2019, 9:29 AM

Remove duplicate run command

hans accepted this revision.Nov 27 2019, 6:45 AM
hans added inline comments.
compiler-rt/test/profile/Windows/instrprof-multiprocess.test
2

I just noticed, this new dir doesn't seem to get used anywhere. I'll remove it when committing.

This revision was automatically updated to reflect the committed changes.