Spurious failure in coverage report test

[RalfJung]

In #83257 (comment), run-make-fulldeps\coverage-reports failed on x86_64-msvc-1 even though the PR just bumped the Miri submodule -- so this has to be a spurious failure, right?

@rust-lang/infra maybe you have a good idea for whom to ping about this.

[RalfJung]

Cc'ing some people who worked on that test: @richkadel @tmiasko

[richkadel]

The MIR pass produced from -Zinstrument-coverage did produce different results, on Windows.

--- expected_show_coverage.doctest.txt	2021-03-18 13:45:52.989271700 +0000
+++ /d/a/rust/rust/build/x86_64-pc-windows-msvc/test/run-make-fulldeps/coverage-reports/coverage-reports/actual_show_coverage.doctest.txt	2021-03-18 14:40:14.025645000 +0000
@@ -74,13 +74,13 @@
    70|      1|/// doctest_crate::fn_run_in_doctests(3);
    71|      1|/// ```
    72|       |///
-   73|      1|fn main() {
-   74|      1|    if true {
-   75|      1|        assert_eq!(1, 1);
+   73|       |fn main() {
+   74|       |    if true {
+   75|       |        assert_eq!(1, 1);
    76|       |    } else {
    77|       |        assert_eq!(1, 2);
    78|       |    }
-   79|      1|}
+   79|       |}

[richkadel]

Submitted that comment prematurely.

I'm putting more comments in #83257 because I think it's just as likely the MIRI changed the MIR that the coverage pass needs to generate coverage.

But it's not clear.

It's also from a doctest, which is something @Swatinem worked on in the last couple of months.

Maybe something changed affecting how doctest works.

[RalfJung]

The MIR pass produced from -Zinstrument-coverage did produce different results, on Windows.

Indeed, but my PR does not change rustc at all, so this cannot be caused by my PR.

[richkadel]

@tmiasko - The timing is very coincidental with your PR landing last night. Based on Ralf's last comment, I would expect other PRs to fail similarly. I don't know how your PR passed, or how this could be spurious, but it is suspect.

If we see more of these failures, you may need to look at reverting the PR. But I think we should make sure this is repeatable first.

@wesleywiser FYI

[tmiasko]

Timing indeed makes that PR suspect. That being said, I don't see how it could introduce any functional changes for builds without MIR inlining, like in this test case.

[richkadel]

Also, @RalfJung's PR succeeded on @bors retry.

Hard to understand how that could have happened, but for now let's assume it was a random and hopefully uncommon issue.

[Mark-Simulacrum]

@richkadel we've seen this several times since then, and it's likely that we'll want to address it either by removing the test or repeatedly running it if it fails, etc. -- do you have any thoughts on how it may have occurred?

The next step is likely trying to run builds in a loop locally, though unfortunately it looks like the issue is somehow tied to MSVC which will make many of the infra team members not have immediate access to machines on that platform I believe...

[richkadel]

Is it always the doc test?

Fyi, @Swatinem if it is.

[Swatinem]

Hm, this might be related to this comment here:

rust/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.doctest.txt

Lines 70 to 75 in 91f2e2d

	60| |//! // this `main` is not shown as covered, as it clashes with all the other
	61| |//! // `main` functions that were automatically generated for doctests
	62| |//! fn main() -> Result<(), SomeError> {
	63| |//! doctest_main()
	64| |//! }
	65| |//! ```

LLVM assumes functions are unique by name. But every doctest has its own main functions which clash. And it seems like they clash with the main function defined in the crate as well.

Not sure on what this depends. But the .profraw files have the pid in their name, so the ordering is pretty random. Might be a reason for these spurious failures.

rust/src/test/run-make-fulldeps/coverage-reports/Makefile

Lines 108 to 110 in 91f2e2d

	"$(LLVM_BIN_DIR)"/llvm-profdata merge --sparse \
	"$(TMPDIR)"/$@-*.profraw \
	-o "$(TMPDIR)"/[email protected]

rust/src/test/run-make-fulldeps/coverage-reports/Makefile

Lines 119 to 125 in 91f2e2d

	$(call BIN,"$(TMPDIR)"/$@) \
	$$( \
	for file in $(TMPDIR)/rustdoc-$@/*/rust_out; \
	do \
	[ -x "$$file" ] && printf "%s %s " -object $$file; \
	done \
	) \

I think the action item here is:

• rename the main function in the crate so it does not clash
• maybe add an explicit coverage-ignore attribute to the main inside of the one doctest, so it does not accidentally have coverage.

[Swatinem]

Or, I think I named the function main specifically to make sure that it overrides any main defined in the doctests. (and because its a binary crate so it needs a main 🙄)
In that case it might be better to just name the .profraw of the main coverage run differently and explicitly order it as the first argument to the merge.

[RalfJung]

That sounds more like a bug in coverage collection/reporting then? Surely that should work properly also when users have multiple functions with the same name (to the extend that Rust allows that).

[Swatinem]

@RalfJung I think that is some kind of logic in llvm that uses only one functions data if multiple functions with the same mangled (C) name are present.

I have been playing around with ordering of CLI parameters and file naming, but wasn’t able to reproduce this.

My hunch is that maybe it is a pid reuse issue, in which case the earlier coverage file is overwritten. I have a PR that tries to reduce the chance of this.

Or maybe llvm is using some kind of timestamp to decide which record to keep and which to discard. I have too little insight to know.

[richkadel]

It looks like @Aaron1011 just hit this same spurious failure, in #85211.

I hadn't heard anything about this issue since @RalfJung first reported it on March 18. I don't know if the rate is accelerating, but I see there were other reports mentioned here, and agree we should do something.

@Swatinem - I don't think MSVC testing of the coverage of doctests adds a lot of additional value. It probably tests the variability in Makefile and shell scripting more than any variability in coverage of doc tests on Windows.

I suggest just disabling this test on MSVC, at least for now, until we someday migrate coverage tests off of Makefiles.

I am out of office today through next Monday (my daughter is graduating from University of Illinois!) and don't have much time to work (and I'm slow to respond, sorry).

@Swatinem - I don't know if you have time, but it may not be hard to add a directive to ignore MSVC for specific coverage tests. I think we have one or two similar directives in the Makefile.

[richkadel]

My hunch is that maybe it is a pid reuse issue, in which case the earlier coverage file is overwritten. I have a PR that tries to reduce the chance of this.

@Swatinem - I think my hunch is similar or the same. I suspect it might have something to do with the OS/shell environment in Windows for MSVC builds. PID reuse, or timing, or file lock, or something that we take for granted on a Unixy environment but doesn't work as consistently on Windows (or at least not how we might expect, coming from more of a Unix background).

That seems more likely than an LLVM bug, to me.

I think migrating off of Makefiles would most likely help (#85009) if our hunches are right.

FYI: @tmandry @wesleywiser

[Enselic]

Triage: Has anyone of you seen this spurious failure in the last say two years? Or can we close this issue as obsolete?