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

[Holman]

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.

[davidxl]

Is it possible to add a test?

[hans]

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]

In D70330#1749324, @hans wrote:

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.

[hans]

lgtm

[hans]

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

[Holman]

Added Windows test case.

[Holman]

In D70330#1751055, @hans wrote:

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]

thanks for the test case.

[hans]

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?

[davidxl]

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

[Holman]

Added test for actual profile merge.

[davidxl]

compiler-rt/test/profile/Windows/instrprof-multiwrite.test

6 ↗

(On Diff #230307)

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

[Holman]

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]

compiler-rt/test/profile/Windows/instrprof-multiwrite.test

6 ↗

(On Diff #230307)

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

[Holman]

Add block counts to test

[davidxl]

lgtm

[hans]

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]

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]

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]

Change tests per review comments

[hans]

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]

Remove duplicate run command

[hans]

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.