Avoid possible filename collision in coverage tests #85283
Conversation
Swatinem
force-pushed
the
ordered-profraw
branch
2 times, most recently
from
8e5a60a to
50c5b39
Compare
|
@Swatinem - Thanks for the PR. I need more time to consider these changes than I have right now (on vacation). Would you be willing to upload a separate change that disables the doctest coverage test on MSVC (as discussed in #83262 (comment)) in the mean time? If you feel strongly about this change, you could ask @tmandry or @wesleywiser to take a look in my place. Otherwise, I'll take a look at this PR next week. |
camelid
added
S-waiting-on-review
|
ping @richkadel might this still be a worthwhile change for right now? Surely, refactoring the tests to run directly via rustbuild/compiletest might be interesting, but not really something I feel like getting into right now. |
|
Sorry for the delay! I'll take a look now. |
There was a problem hiding this comment.
@Swatinem - Thanks for the PR!
I left some inline comments. In my view, the only change that I think has a chance of resolving the issue (if our theory is right about reused PIDs) is adding the %m.
Please let me know if you see things differently, and add comments to the Makefile that explain the reasons for options you are adding and the expected impact/benefit of those options (for future reviewers).
Also, I don't understand the PR title, particularly, "Order profraw files explicitly". (I thought this was about filename collisions, and I don't see anything in your change that has to do with ordering files.)
What I think is happening (best guess) is the .profraw files are generated for each doc test, and at least some of the doctests are run under the same Windows PID. If that's happening, then some doc test .profraw results are overwritten, so the coverage for them is lost.
That would happen before running llvm-profdata, so adding --num-threads to the LLVM commands doesn't seem relevant to me.
So I may not fully understand what your change is doing, but if you agree with what I've said, then I recommend changing the title to be more accurate, something like, "Address possible PID-based profraw filename collisions on doc tests".
| @@ -87,7 +87,7 @@ endif | |||
| # Run it in order to generate some profiling data, | |||
| # with `LLVM_PROFILE_FILE=<profdata_file>` environment variable set to | |||
| # output the coverage stats for this run. | |||
| LLVM_PROFILE_FILE="$(TMPDIR)"/$@-%p.profraw \ | |||
| LLVM_PROFILE_FILE="$(TMPDIR)"/$@.profraw \ | |||
There was a problem hiding this comment.
👍 I support the change on this line (removing pid from the non-doc-test run), even though it shouldn't make a difference in the result.
| @@ -98,28 +98,29 @@ endif | |||
| ) | |||
|
|
|||
| # Run it through rustdoc as well to cover doctests | |||
| LLVM_PROFILE_FILE="$(TMPDIR)"/$@-%p.profraw \ | |||
| LLVM_PROFILE_FILE="$(TMPDIR)"/$@-%p-%m.profraw \ | |||
There was a problem hiding this comment.
I think this will help, assuming our theory is correct that there could be PID-reuse on Windows. As I understand it, every doc test produces a new, program (new main() and unique source) so the binary signature (%m) should be different for each one.
In theory, adding the PID %p value is probably superfluous, because %m should be unique enough, but maybe keeping %p is an added safeguard to ensuring we don't have filename collisions, and there's probably harm keeping both.
|
r? @tmandry (I don't have privileges to approve in rust-lang/rust, but Tyler can.) |
Previously, coverage tests were writing profiler data to files based on their pid. As rustdoc spawns each doctest as its own process, it might be possible in rare cases that a pid is being reused which would cause a file to be overwritten, leading to incorrect coverage results.
Swatinem
force-pushed
the
ordered-profraw
branch
from
50c5b39 to
084794e
Compare
|
r? @tmandry I rebased and added another comment, removing unrelated changes. (I was trying different things but couldn’t reproduce this) Will change the PR title and description in a moment. |
Swatinem
changed the title
|
@bors r+ rollup |
|
📌 Commit 084794e has been approved by |
bors
added
S-waiting-on-bors
Rollup of 8 pull requests Successful merges: - rust-lang#85283 (Avoid possible filename collision in coverage tests) - rust-lang#86200 (Updates `Clone` docs for `Copy` comparison.) - rust-lang#86209 (fix minor wording/typo issues in core::option docs) - rust-lang#86242 (rustdoc- dont ICE on `ConstEvaluatable` predicates) - rust-lang#86280 (Add a regression test for issue-76510) - rust-lang#86293 (Allow to run only a few GUI tests) - rust-lang#86327 (Don't mark "safe" intrinsics as unsafe) - rust-lang#86345 (Remove some duplicate `char` assoc items on RELEASES.md) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Previously, coverage tests were writing profiler data to files based on
their pid. As rustdoc spawns each doctest as its own process, it might
be possible in rare cases that a pid is being reused which would cause
a file to be overwritten, leading to incorrect coverage results.
should help with #83262
r? @tmandry