Coverage tests for remaining TerminatorKinds and async, improve Assert #79109

richkadel · 2020-11-16T19:43:55Z

Tested and validate results for panic unwind, panic abort, assert!()
macro, TerminatorKind::Assert (for example, numeric overflow), and
async/await.

Implemented a previous documented idea to change Assert handling to be
the same as FalseUnwind and Goto, so it doesn't get its own
BasicCoverageBlock anymore. This changed a couple of coverage regions,
but I validated those changes are not any worse than the prior results,
and probably help assure some consistency (even if some people might
disagree with how the code region is consistently computed).

Fixed issue with async/await. AggregateKind::Generator needs to be
handled like AggregateKind::Closure; coverage span for the outer async
function should not "cover" the async body, which is actually executed
in a separate "closure" MIR.

rust-highfive · 2020-11-16T19:43:59Z

r? @oli-obk

(rust_highfive has picked a reviewer for you, use r? to override)

richkadel · 2020-11-16T19:44:08Z

r? @tmandry

richkadel · 2020-11-16T19:44:58Z

fyi: @wesleywiser

richkadel · 2020-11-16T20:09:52Z

! ... I forgot to bless at least one of the new tests. I'm building the blessed results now and will push an update.

Then I also plan to pull this PR down to my Mac and to my Windows VM so I can make sure (as best as I can) that these tests work on all 3 platforms (including Linux, my main dev environment).

richkadel · 2020-11-16T20:13:20Z

Oh, and I just realized I should rebase with the latest changes (including the 2 PRs that just landed for coverage). They may not affect this PR, but I need to be sure.

richkadel · 2020-11-16T22:44:26Z

new tests are blessed and pass on Linux... I'll be testing Mac and Windows next.

richkadel · 2020-11-17T02:24:14Z

FYI, something seems broken in the rustc build. After rebasing today, and then pulling down this PR to MacOS and Windows local environments, neither of those will build the profiler_builtins library anymore.

https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/profiler_builtins.20not.20compiling.20on.20MacOS.20or.20Windows

I "think" it might still work on Linux, but I'm a little afraid to try a clean build at this point 😨

richkadel · 2020-11-17T03:17:57Z

Issue #79124 affects local builds of this PR

richkadel · 2020-11-17T04:26:37Z

Tests are working on Mac. Windows test is taking forever... ;-)

richkadel · 2020-11-17T06:41:07Z

Windows tests pass as well.

@tmandry - subject to any review feedback changes requested, this PR should be good to merge.

Thanks!

tmandry

Switching to focusing on test behavior now.

I think the most important tweak would be to focus on how braces behave, and try to make them as consistent as possible across syntactic forms.

tmandry · 2020-11-17T23:50:49Z

src/test/run-make-fulldeps/coverage-reports-base/expected_show_coverage.abort.txt

+   16|     11|    while countdown > 0 {
+   17|     10|        if countdown < 5 {
+   18|      4|            might_abort(false);
+   19|      6|        }


As a data point, I was confused by the count on this line, since I was expecting to match either of the above two lines.

I'm sure this pre-exists this PR but I hadn't noticed it until now.

This is actually correct.

The condition countdown < 5 executed 10 times (10 loop iterations).

It evaluated to true 4 times, and executed the might_abort() call.

It skipped the body of the might_abort() call 6 times (hidden else).

I wanted to capture the coverage of the non-true condition. Say the condition was countdown < 50, which is always true. In that case, we wouldn't have a test for what happens if might_abort() is not called.

So counting the "hidden else" is important.

I can clarify this, but I should probably clarify it in a different test that's more focused on if blocks inside loops, and keep this test focused on testing the Abort terminator.

One side-note... The Span for the "hidden else" starts as a 0-length span right after the right brace, if I'm not mistaken. But code region counts versus line counts are confusing for 0-length regions, so I always expend them by 1 character, preferably to the right (in the typical case, after the brace) UNLESS it's at the end of the line, in which case I have to expand it to the left (which is the right brace).

You can't see it because there are no more code regions on that line, and the code block ends at the semicolon after the might_abort() call.

If there was no semicolon (say, if might_abort() returned a value), then there would be multiple code regions on that line, and you would see the one character highlighted. I'm not sure that would help clarify things anymore than in it's existing state though.

Yeah, I was able to infer what it was doing, I just found it confusing at first. Your explanation was helpful to understand why doing this is a good idea, thanks.

I went ahead and expanded this example, and added comments and notes to help explain the counter for the implicit else.

Great news here! I changed how the Span is set for implicit else when lowering from AST, and it works! No more special case for Goto. Lots of related issues resolved.

tmandry · 2020-11-17T23:58:17Z

src/test/run-make-fulldeps/coverage-reports-base/expected_show_coverage.async.txt

+   11|       |        y if f().await == y => (),
+   12|       |        _ => (),
+   13|       |    }
+   14|      1|}


If I had a magic wand, I think the ideal would be for the function signature to count calls..? Or possibly the opening and (optionally) closing brace. I'm not sure which would be more intuitive.

Technically when you call an async fn you aren't running any of its code, so counting the braces might still be incorrect, but as long as the user can understand and reason about what happened I'm not especially concerned about that.

I think closing brace only is not very intuitive.

I think I'd like to count the opening brace or signature, as you suggest. I'll look into that.

There's a second issue here:

Why do the generator bodies show no counts? (Not even 0.)

I need to understand that and see if it can be fixed.

If I counted the bodies, then we SHOULD see something like:

5| 1|async fn f() -> u8 { 1 } ^0

As it is, though, there's no way to tell that this async function was actually NOT executed.

Interesting. I wonder if the counters are getting optimized out somehow.

Partial correction. The body of f() actually was executed (because the test blocks on i(), and i() awaits f().

But g() is a similar example of the point I was trying to make.

From the debug logs, I can see the generator/closure MIR for the body of g() is never passed to Rust's codegen phase.

(FYI, -Clink-dead-code does not make a difference either; I think that flag is only used by the target linker.)

I think I may have looked into this (and if so, it was a hard problem) but it would be nice to find any MIR's skipped at codegen, and still inject something in the coverage map for the function body to, hopefully, produce line counts of 0 for the body.

@tmandry @wesleywiser - I'm a little unsure how to proceed here.

I'll try my best to summarize my understanding and what I think I want to do but don't know how:

Given a Crate, rustc_mir::monomorphize::collector finds the "root" DefIds (via the RootCollector, a HIR ItemLikeVisitor), and traverses them to generate a set of MonoItems. This is where the async closures (Generators) are getting dropped if they are not going to be called. (From debug logs, i::{closure#0}() and f::{closure#0}() are both logged, because they are going to be called, but the other async functions' closures are not mentioned at all. There is no "path" from the roots (only main() in this case) to those closures.

It seems to me I can make another ItemLikeVisitor, find all of the hir::ItemKindFns, get their DefIds, and look them up in the set of MonoItems. If a DefId is not found among the MonoItems, then it will not be codegen'ed, and therefore won't have coverage.

At that point, I want to add at least one Zero counter with span (code region) for the span of the closure/generator, for each missing DefId, to the rustc_codegen_ssa::coverageinfo::map::FunctionCoverage (the collection of counters and coderegions built up during the codegen phase, and eventually written out as the "Coverage Map". I would do this with FunctionCoverage::add_counter(self, counter, region).

Here's where I get confused...

The FunctionCoverage is stored in a hash map in CrateCoverageContext, and that object is constructed as a field in CodegenCx, but CodegenCx appears to be constructed for a specific CodegenUnit

IIUC, codegen of a given crate can be partitioned across multiple CodegenUnits, and each CodegenUnit will have only a subset of the crate's MonoItems; so if that's true:

Given a DefId in some Crate, even if the DefId is not be found among the MonoItems of one CodegenUnit, the DefId might still be found among the MonoItems of one of the otherCodegenUnits for that Crate. If so, that prevents me being able to check for missing DefIds when I'm starting to codegen the Coverage Map, because the Coverage Map finalization happens within the CodegenCx of only one CodegenUnit.

But if I search for a DefId among all MonoItems of all CodegenUnits, and don't find it, how do I know which CodegenCx to use, to add the missing Counter and code region for that function?

What's even more baffling to me is, it looks to me like a CoverageMap is codegenned for each CodegenCx, and assuming there are more than one, then the code appears to be set up to generate multiple coverage maps. I don't really get how that would work (or why it appears to work now), but I assume it does work since evidence suggests as much.

So I guess I just don't understand something, and hopefully the solution (to adding counters and code regions for functions/closures that are missing from the MonoItems is easier than it appears.

The discussion continued at https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/counters.20for.20uncalled.20functions

I couldn't simply inject code regions for functions not codegenned, as discussed in the PR for my attempted changes in draft PR #79392, but I was able to make other changes to improve this.

I'm now adding a coverage code region for the function signature, up to (but not including) the opening brace. The existing code for combining spans helps simplify (I think in a good way) how the function signature span can continue into the function body, if no other spans interfere. I think the results look good.

I also addressed showing 0 coverage for closures and async function bodies by injecting what the Coverage Mapping Format calls a "gap region". There are additional notes on this, and some non-intuitive results (but not wrong) occasionally. Given the trade-offs, this is better for ensuring missing coverage is identified.

And, as I showed in #79109 (comment), -C link-dead-code does make a difference for some unused functions. (Just pointing that out, since I discussed it here as well.)

I made a different change, backing out my original approach to closures, and I'm now adding coverage for any unused MIR. This addresses missing coverage for async functions, closures, and unused regular functions.

tmandry · 2020-11-18T00:02:08Z

src/test/run-make-fulldeps/coverage-reports-base/expected_show_coverage.async.txt

+   25|      1|async fn i(x: u8) {
+   26|      1|    match x {
+   27|      1|        y if f().await == y + 1 => (),
+                      ^0                         ^0


Does the y being uncovered also happen in non-async code? I think I remember you mentioning something like this. It's not wrong if you consider that it's the binding of y. It is a bit unintuitive, though.

Yes, this is the same for async and non-async.

Yes it was discussed because it is less intuitive. At one point, I tried to include the y in the pattern code region, but the logic to allow that (which may seem more intuitive, but is counter to what the MIR says) caused problems handling other cases.

I decided it was better to honor the MIR in all cases and show what's really happening.

This is not just one of those situations where we can be accurate but non-intuitive, or intuitive but inaccurate; the other problem is, it would require special case handling in the coverage code (which is something we both prefer to avoid). 😄

Yeah, special casing is not great. We can always change the way MIR building emits source info at some point.

tmandry · 2020-11-18T00:05:30Z

src/test/run-make-fulldeps/coverage-reports-base/expected_show_coverage.async.txt

+   59|      1|            if let Poll::Ready(val) = future.as_mut().poll(&mut context) {
+   60|      1|                break val;
+   61|       |            }
+   62|      0|        }


This would cause tooling to complain about missing coverage of this line, but the break does happen.

Would it be possible to make all loop exits count the closing loop brace?

@tmandry - I'm confused by your interpretation on this one.

To me, this is intuitive. The bottom of the loop is never reached.

This is a coverage gap because the program does not test an iteration of the loop when it does not break.

Oh, now I see. That makes sense.

tmandry · 2020-11-18T00:22:09Z

src/test/run-make-fulldeps/coverage-reports-base/expected_show_coverage.conditions.txt

+   40|       |        // Demonstrate the difference with `TerminatorKind::Assert` as of 2020-11-15. Assert is no
+   41|       |        // longer treated as a `BasicCoverageBlock` terminator, which changed the coverage region,
+   42|       |        // for the executed `then` block above, to include the closing brace on line 30. That
+   43|       |        // changed the line count, but the coverage code region (for the `else if` condition) is
+   44|       |        // still valid.
+   45|       |        //
+   46|       |        // Note that `if` (then) and `else` blocks include the closing brace in their coverage
+   47|       |        // code regions when the last line in the block ends in a semicolon, because the Rust
+   48|       |        // compiler inserts a `StatementKind::Assign` to assign `const ()` to a `Place`, for the
+   49|       |        // empty value for the executed block. When the last line does not end in a semicolon
+   50|       |        // (that is, when the block actually results in a value), the additional `Assign` is not
+   51|       |        // generated, and the brace is not included.
+   52|      1|        let mut countdown = 0;


Yeah, this is pretty unintuitive. "Closing brace is counted when the scope is exited, or skipped by if/loop condition," seems like the most logical behavior to me.

"Closing brace is counted when the scope is exited, or skipped by if/loop condition,"

@tmandry are you quoting something here, or just suggesting a way you'd like it described?

On the one hand, counting the closing brace when exiting the scope is how we currently count the closing brace of a function, so this would be more consistent with that.

An argument could be made that I shouldn't count the closing brace of a function, though, if the function was exited by an early return.

The truth is, in some cases, the reason things work the way they do for functions and for loops is because it was the best I could do given the MIR, and seemed "reasonable" (even if there were alternatives like these).

Personally, I like the idea of changing how function end braces are counted, rather than changing how loop end braces are counted. I think it's more intuitive to count the end brace only if the code did not exit the scope earlier (by break, continue, or return).

Either way, it's inconsistent now.

Should we continue debating this here? Or log it as an unresolved new "Issue" and invite the community for comment?

If the latter, maybe I shouldn't change either one, for now, since it could be a waste of time if we decide to go a different way.

Yeah I think there are a few competing principles..

Consistency

Intuitiveness

Preserving all information

and I'm not exactly sure how to resolve them. My quote was one idea I had.

This might be good to hash out in an issue somewhere.

I think a lot of things like this are resolved with my most recent improvements.

tmandry · 2020-11-18T00:27:47Z

src/test/run-make-fulldeps/coverage-reports-base/expected_show_coverage.panic_unwind.txt

+   45|       |//   6. By best practice, programs should not panic. By design, the coverage implementation will not
+   46|       |//      incur additional cost (in program size and execution time) to improve coverage results for
+   47|       |//      an event that is not supposted to happen.


I don't think this is always true. Of course there are uses like when invariants are violated (bugs), and #[should_panic] tests which you mention below. In addition, salsa supports unwinding as a cancellation mechanism for queries.

Point taken. It's still my opinion that the best practice is not to use panic! as a programming model. (Even salsa appears to support both panic and Result::Err for cancelling, if I'm not mistaken. I'm curious why not just go with the Result::Err method, unless it's for legacy reasons.)

But since this may be more my opinion, I'll rephrase.

I think they support unwinding because otherwise cancellation becomes "infectious" and something you have to handle throughout your code, even though it isn't a classic error handling case. I think rust-analyzer uses it.

I'm updating the comment.

src/test/run-make-fulldeps/coverage-reports-base/expected_show_coverage.yield.txt

tmandry · 2020-11-18T00:47:43Z

src/test/run-make-fulldeps/coverage-reports-deadcode/expected_show_coverage.assert.txt

@@ -0,0 +1,34 @@
+    1|       |#![allow(unused_assignments)]


I'm not seeing how these -deadcode test files are any different, or add value. I guess that's one piece of the cleanup to be done.

The -Clink-dead-code defaults to on on Linux and Mac, and off on Windows. If the results are different, the tests would fail on Windows if I only had one variation.

I wasn't sure if I'd get different coverage results depending on the flag. Now that I have several examples, I'll diff them to see. It's a little risky to assume the flag makes no difference, even if true for the current tests.

@tmandry - FYI, currently, two tests have slightly different coverage results.

$ diff -r coverage-reports-base coverage-reports-deadcode -x '*counters*' -x '*.json' -x 'prett*' -x 'Make*'

15c15 < 15| |async fn e() -> u8 { 1 } --- > 15| 0|async fn e() -> u8 { 1 } 19c19 < 19| |async fn foo() -> [bool; 10] { [false; 10] } --- > 19| 0|async fn foo() -> [bool; 10] { [false; 10] } diff --color -r -x '*counters*' -x '*.json' -x 'prett*' -x 'Make*' '--color=never' coverage-reports-base/expected_show_coverage.partial_eq.txt coverage-reports-deadcode/expected_show_coverage.partial_eq.txt 5a6,15 > ------------------ > | Unexecuted instantiation: <partial_eq::Version as core::cmp::PartialOrd>::gt > ------------------ > | Unexecuted instantiation: <partial_eq::Version as core::cmp::PartialOrd>::le > ------------------ > | Unexecuted instantiation: <partial_eq::Version as core::cmp::PartialOrd>::ge > ------------------ > | <partial_eq::Version as core::cmp::PartialOrd>::lt: > | 4| 1|#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] > ------------------ 8a19,28 > ------------------ > | Unexecuted instantiation: <partial_eq::Version as core::cmp::PartialOrd>::gt::{closure#0} > ------------------ > | Unexecuted instantiation: <partial_eq::Version as core::cmp::PartialOrd>::ge::{closure#0} > ------------------ > | <partial_eq::Version as core::cmp::PartialOrd>::lt::{closure#0}: > | 7| 1| minor: usize, > ------------------ > | Unexecuted instantiation: <partial_eq::Version as core::cmp::PartialOrd>::le::{closure#0} > ------------------

deadcode is no longer applicable to coverage! I'm able to instrument the deadcode functions without using the -Clink-dead-code option. Special tests are now gone.

richkadel · 2020-11-18T16:30:05Z

I think the most important tweak would be to focus on how braces behave, and try to make them as consistent as possible across syntactic forms.

@tmandry I think there may be at least one or two improvements I can make, but while I do agree with you that some of the results are hard to interpret, there are cases that are both hard to interpret and still correct. (Maybe I can't make them any easier to interpret without making the coverage results inaccurate or incomplete.)

In those cases, I don't think we should sacrifice coverage accuracy/completeness just to make the results "appear" more intuitive (and I think you'd agree), but I also don't know if there's much I can do to make them more clear.

Maybe add comments at points in the examples where we see confusing results?

richkadel · 2020-11-18T16:37:47Z

src/test/run-make-fulldeps/coverage-reports-base/expected_show_coverage.panic_unwind.txt

+   17|      0|            might_panic(true);
+   18|     10|        } else if countdown < 5 {
+   19|      3|            might_panic(false);
+   20|     15|        }


I just noticed this incorrect line count, 15, here and at the same point in the panic_unwind.rs test.

This is wrong of course.

I suspect I have overlapping coverage regions with different counters, and they are adding up. (Best guess for now.) I'll look into it.

I think it may be related to how I implement the "hidden else", discussed in my previous reply to @tmandry in abort.rs. I may need to rethink what to do if the span is at the end of a line, and possible expand to the first character in the next line, if not at the end of the body.

(FYI, the expansion to the left is actually what I want for the end of the body. This is how I count function Returns. So I don't want to break that.)

I fixed this when I fixed the Goto implicit else span, and remove the special case handling of Goto (which didn't work that well in some cases... like this one).

tmandry · 2020-11-18T18:33:20Z

@bors delegate+

bors · 2020-11-18T18:33:23Z

✌️ @richkadel can now approve this pull request

bors · 2020-11-26T16:53:52Z

☔ The latest upstream changes (presumably #79441) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

richkadel · 2020-11-29T09:20:41Z

Some significant coverage count improvements while also simplifying the code a bit, removing some special case handling.

richkadel · 2020-11-30T05:15:04Z

I learned some things while reviewing these changes, and managed to simplify things even further, removing the benefits of enforcing -Clink-dead-code. That means I have no need to test differently on MSVC, so I was able to remove the duplicate tests (testing link-dead-code on and off). So I removed half of the run-make-fulldeps/coverage-* tests.

I did run into a couple of weird anomalies, which I had to account for in the coverage-reports Makefile (just to make sure the output stayed consistent, for now). Adding --emit llvm-ir, for some reason, changes the output now (or rather, if I don't add that option, the output is a bit different compared to how it was before this commit). It only affects a couple of tests (async.rs and closures.rs, I believe, both of which have closures).

The other is, a couple of tests create coverage results from built-in Rust macros, that now show up in llvm-cov show results. I added a filename filter to exclude them. The results don't appear helpful, but they aren't wrong, and it's not unusual to filter some boilerplate from coverage results.

I'm assuming that the new procedure in mapgen.rs for adding missing coverage causes both of these issues, but I don't know why at this time.

richkadel · 2020-11-30T05:24:21Z

@tmandry - Although you gave me permission to approve this already, I do think the changes here are substantial enough to warrant another look.

Good news is, I removed more than I added (in the normal code base) AND removed half of the tests (due to removing the need for -Clink-dead-code.

Can you take another look?

bors · 2020-12-03T16:49:04Z

⌛ Testing commit 2c99600a8679615f601aa010825012e9b46fe59b with merge cfd64d0875173833732b8c0b2c9ca92bc0d0b1bb...

Tested and validate results for panic unwind, panic abort, assert!() macro, TerminatorKind::Assert (for example, numeric overflow), and async/await. Implemented a previous documented idea to change Assert handling to be the same as FalseUnwind and Goto, so it doesn't get its own BasicCoverageBlock anymore. This changed a couple of coverage regions, but I validated those changes are not any worse than the prior results, and probably help assure some consistency (even if some people might disagree with how the code region is consistently computed). Fixed issue with async/await. AggregateKind::Generator needs to be handled like AggregateKind::Closure; coverage span for the outer async function should not "cover" the async body, which is actually executed in a separate "closure" MIR.

In preparation for removing the -deadcode variants

Fixes multiple issue with counters, with simplification Includes a change to the implicit else span in ast_lowering, so coverage of the implicit else no longer spans the `then` block. Adds coverage for unused closures and async function bodies. Fixes: rust-lang#78542 Adding unreachable regions for known MIR missing from coverage map Cleaned up PR commits, and removed link-dead-code requirement and tests Coverage no longer depends on Issue rust-lang#76038 (`-C link-dead-code` is no longer needed or enforced, so MSVC can use the same tests as Linux and MacOS now) Restrict adding unreachable regions to covered files Improved the code that adds coverage for uncalled functions (with MIR but not-codegenned) to avoid generating coverage in files not already included in the files with covered functions. Resolved last known issue requiring --emit llvm-ir workaround Fixed bugs in how unreachable code spans were added.

Added one more test (two files) showing coverage of generics and unused functions across crates. Created and referenced new Issues, as requested. Added comments. Added a note about the possible effects of compiler options on LLVM coverage maps.

The original test produced a single crate with two mods, which was not the goal of the test.

bors · 2020-12-03T18:54:53Z

💔 Test failed - checks-actions

richkadel · 2020-12-03T20:02:52Z

@bors retry

richkadel · 2020-12-03T20:03:45Z

See https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/x86_64-apple-darwin.20CI.20randomly.20injecting.20typos.20in.20Makefile.3F

richkadel · 2020-12-03T20:17:15Z

@bors r=tmandry

bors · 2020-12-03T20:17:16Z

📌 Commit dc4bd90 has been approved by tmandry

yaahc · 2020-12-03T21:33:41Z

src/doc/unstable-book/src/compiler-flags/source-based-code-coverage.md

+    target/debug/deps/lib-30768f9c53506dc5 \
+    target/debug/deps/json5format-fececd4653271682


my experience in ZcashFoundation/zebra#1293 indicates that this will not produce the proper coverage report. I didn't quite get to the bottom of exactly what was happening but when I tried specifying the bins as a list of space separated arguments it would lose some of the coverage reports, and ordering mattered. My guess is it was ignoring everything after the first, or overwriting them somehow. In order to fix it I had to prefix every bin with -object.

ZcashFoundation/zebra@f4b1cd2 This is the PR where I did it, the fix being adding the printf when we output the file names.

It might also be a good idea to suggest how to extract the list of binaries that were ran by cargo-test, or link to examples that do so.

cargo test --no-run --message-format=json \ | jq -r "select(.profile.test == true) | .filenames[]" \ | grep -v dSYM -

Cool! Thanks for the recommendation. I've been struggling to get this PR to land so I may apply these suggestions in a follow-up PR.

I've addressed this in #79818.

Thanks again!

bors · 2020-12-04T02:31:17Z

⌛ Testing commit dc4bd90 with merge 6513f50...

bors · 2020-12-04T04:51:40Z

☀️ Test successful - checks-actions
Approved by: tmandry
Pushing 6513f50 to master...

rust-highfive assigned oli-obk Nov 16, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 16, 2020

rust-highfive assigned tmandry and unassigned oli-obk Nov 16, 2020

richkadel changed the title ~~Add tests for some TerminatorKinds and change Assert handling~~ Coverage tests for remaining TerminatorKinds and async, improve Assert Nov 16, 2020

richkadel force-pushed the llvm-coverage-counters-2.0.5 branch from d6936f5 to ba60b3e Compare November 16, 2020 22:39

richkadel mentioned this pull request Nov 17, 2020

Tracking Issue for LLVM InstrProf code coverage (current flag: -Zinstrument-coverage) #79121

Closed

3 tasks

richkadel force-pushed the llvm-coverage-counters-2.0.5 branch from ba60b3e to 48ab271 Compare November 17, 2020 04:58

tmandry reviewed Nov 18, 2020

View reviewed changes

richkadel commented Nov 18, 2020

View reviewed changes

richkadel force-pushed the llvm-coverage-counters-2.0.5 branch from 48ab271 to 311b635 Compare November 29, 2020 09:19

richkadel force-pushed the llvm-coverage-counters-2.0.5 branch from 311b635 to 6ee4a41 Compare November 29, 2020 16:33

richkadel mentioned this pull request Nov 29, 2020

MIR InstrumentCoverage - Can spans for TerminatorKind::Goto be improved to avoid special cases? #78542

Closed

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 3, 2020

richkadel added 6 commits December 3, 2020 09:50

Removed -base from run-make-fulldeps/coverage-*-base

f6c9c1a

In preparation for removing the -deadcode variants

Addressed feedback from 2020-12-01

d96f351

Added one more test (two files) showing coverage of generics and unused functions across crates. Created and referenced new Issues, as requested. Added comments. Added a note about the possible effects of compiler options on LLVM coverage maps.

Workaround for inconsistent order of llvm-cov results on Windows

b0c140a

Fixed cross-crate generic call test to compile lib and bin separately

f101fd8

The original test produced a single crate with two mods, which was not the goal of the test.

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 3, 2020

Tweak to Makefile to overcome MacOS make corruption bug

dc4bd90

richkadel force-pushed the llvm-coverage-counters-2.0.5 branch from 2c99600 to dc4bd90 Compare December 3, 2020 20:01

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 3, 2020

yaahc reviewed Dec 3, 2020

View reviewed changes

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 4, 2020

bors merged commit 6513f50 into rust-lang:master Dec 4, 2020

rustbot added this to the 1.50.0 milestone Dec 4, 2020

This was referenced Dec 4, 2020

Use probe-stack=inline-asm in LLVM 11+ #77885

Merged

Reintroduce hir::ExprKind::If #79328

Merged

jschwe mentioned this pull request Jan 26, 2021

Bump criterion from 0.3.3 to 0.3.4 hermit-os/uhyve#72

Merged

richkadel mentioned this pull request Mar 8, 2021

Invalid LLVM coverage data produced when compiled with -C opt-level=1 #82144

Closed

		target/debug/deps/lib-30768f9c53506dc5 \
		target/debug/deps/json5format-fececd4653271682

Coverage tests for remaining TerminatorKinds and async, improve Assert #79109

Coverage tests for remaining TerminatorKinds and async, improve Assert #79109

Conversation

richkadel commented Nov 16, 2020 • edited Loading

rust-highfive commented Nov 16, 2020

richkadel commented Nov 16, 2020

richkadel commented Nov 16, 2020

richkadel commented Nov 16, 2020

richkadel commented Nov 16, 2020

richkadel commented Nov 16, 2020

richkadel commented Nov 17, 2020

richkadel commented Nov 17, 2020

richkadel commented Nov 17, 2020

richkadel commented Nov 17, 2020

tmandry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richkadel Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richkadel Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richkadel commented Nov 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmandry commented Nov 18, 2020

bors commented Nov 18, 2020

bors commented Nov 26, 2020

richkadel commented Nov 29, 2020

richkadel commented Nov 30, 2020

richkadel commented Nov 30, 2020 • edited Loading

bors commented Dec 3, 2020

bors commented Dec 3, 2020

richkadel commented Dec 3, 2020

richkadel commented Dec 3, 2020

richkadel commented Dec 3, 2020

bors commented Dec 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Dec 4, 2020

bors commented Dec 4, 2020

richkadel commented Nov 16, 2020 •

edited

Loading

richkadel Nov 18, 2020 •

edited

Loading

richkadel Nov 26, 2020 •

edited

Loading

richkadel commented Nov 30, 2020 •

edited

Loading