Working expression optimization, and some improvements to branch-level source coverage #78267

richkadel · 2020-10-23T08:59:00Z

This replaces PR #78040 after reorganizing the original commits (by request) into a more logical sequence of major changes.

Most of the work is in the MIR transform/coverage/ directory (originally, transform/instrument_coverage.rs).

Note this PR includes some significant additional debugging capabilities, to help myself and any future developer working on coverage improvements or issues.

In particular, there's a new Graphviz (.dot file) output for the coverage graph (the BasicCoverageBlock control flow graph) that provides ways to get some very good insight into the relationships between the MIR, the coverage graph BCBs, coverage spans, and counters. (There are also some cool debugging options, available via environment variable, to alter how some data in the graph appears.)

And the code for this Graphviz view is actually generic... it can be used by any implementation of the Rust Graph traits.

Finally (for now), I also now output information from llvm-cov that shows the actual counters and spans it found in the coverage map, and their counts (from the --debug flag). I found this to be enormously helpful in debugging some coverage issues, so I kept it in the test results as well for additional context.

@tmandry @wesleywiser

r? @tmandry

Here's an example of the new coverage graph:

Within each BasicCoverageBlock (BCB), you can see each CoverageSpan and its contributing statements (MIR Statements and/or Terminators)
Each CoverageSpan has a Counter or and Expression, and Expressions show their Add/Subtract operation with nested operations. (This can be changed to show the Counter and Expression IDs instead, or in addition to, the BCB.)
The terminators of all MIR BasicBlocks in the BCB, including one final Terminator
If an "edge counter" is required (because we need to count an edge between blocks, in some cases) the edge's Counter or Expression is shown next to its label. (Not shown in the example below.) (FYI, Edge Counters are converted into a new MIR BasicBlock with Goto)

r? @tmandry
FYI: @wesleywiser

richkadel · 2020-10-23T09:08:20Z

@tmandry - I reorganized the commits in the most logical way I could come up with. Does this work for you?

If so, this is ready for your review, and other than feedback changes, it's ready to merge, as far as I'm concerned. (No more changes planned, to this PR.)

@wesleywiser - Just FYI, I'm taking Mon-Wed off next week so I may not respond right away, after Saturday, if you are sending feedback as well.

Thanks!

bors · 2020-10-24T18:55:22Z

☔ The latest upstream changes (presumably #78319) 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-10-24T22:09:35Z

I resolved the conflicts introduced by my bugfix PR #78300

tmandry

Just got through the first commit today.

I don't remember if I've asked this before, but wanted to check that unwind edges still split BCBs. I know we don't add counters for them, but we still need to make sure that code after a call isn't counted if the call panics.

Is there a way we can only add the html test output if it's really adding something to the test? I suspect it's redundant in many cases. I'm certainly not reading through it in each diff :)

Finally, food for thought: Could some of the things checked by these end-to-end tests actually be checked by unit tests in the code? Unit tests aren't too common in the compiler today, but with as much intricate logic as there is in this code, I think they're worth considering.

compiler/rustc_codegen_ssa/src/coverageinfo/map.rs

compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs

compiler/rustc_middle/src/mir/coverage.rs

compiler/rustc_mir/src/transform/instrument_coverage.rs

tmandry · 2020-10-24T23:42:54Z

compiler/rustc_mir/src/transform/instrument_coverage.rs

        | TerminatorKind::FalseEdge { .. } => None,

+        // FIXME(richkadel): Note that `Goto` was initially filtered out (by returning `None`, as


Might be worth opening an issue to track all the kinds of "span weirdness" encountered and collect FIXMEs like this. I'd like to see some discussion about what we could change in MIR building / other upstream code.

Got it. I'll create an issue for this (and look for others to add) and add Issue references in the code once they are created.

Issue #78542 (I'm adding this to the comment)

compiler/rustc_mir/src/transform/instrument_coverage.rs

richkadel · 2020-10-25T05:36:05Z

I don't remember if I've asked this before, but wanted to check that unwind edges still split BCBs. I know we don't add counters for them, but we still need to make sure that code after a call isn't counted if the call panics.

I'm adding a second call() right after the first call(true) in the test try_error_result.rs, to show this works for managed errors:

rust/src/test/run-make-fulldeps/coverage/try_error_result.rs

Lines 24 to 32 in a1dfd24

    
           if 
        
               countdown < 5 
        
           { 
        
               call(/*return_error=*/ true)?; 
        
           } 
        
           else 
        
           { 
        
               call(/*return_error=*/ false)?; 
        
           }

Here's the result:

The coverage results are exactly what I would expect: The ? after call(true) (which returns Err) is not highlighted in red here, which shows the error handler was executed (1 time; the same number of times as the call itself). The call(false) immediately after that (before else) is not covered (the entire line is red, and the count is 0), which is correct.

I remember discussing unmanaged errors a while back, and here's my position again (which I still think is right): For the initial release, if a program panics, I believe the results will probably be the same results one might expect for a clang program that crashes: that is, they are not guaranteed to be accurate, and I'm not making any guarantees.

If someone thinks we should support accurate coverage results for programs that panic, we can make it a feature request.

For now, just trying this in the try_error_result.rs test, replacing the Err() return status with panic!(), the result they get is (per my current approach):

$ LLVM_PROFILE_FILE=try_error_result.profraw ./try_error_result
thread 'main' panicked at 'explicit panic', src/test/run-make-fulldeps/coverage/try_error_result.rs:6:9
stack backtrace:
   0:     0x5608e9f59e31 - std::backtrace_rs::backtrace::libunwind::trace::he2381431404d861d
                               at /usr/local/google/home/richkadel/rust/library/std/src/../../backtrace/src/backtrace/libunwind.rs:100:5

To your question, I don't inject separate counters for each function call in case of unwind, so depending on where the counter is incremented, it may show all non-branching statements (including function calls) as having been executed, or non of them, even thought a panic!() may interrupt the sequence somewhere in the middle.

It would be inefficient to inject separate counters and coverage regions for each call, just in case one of them panics. It could be done, but I don't think it's worth doing in this first version.

That's still my position.

Are you OK with that for this release?

richkadel · 2020-10-25T06:29:59Z

Is there a way we can only add the html test output if it's really adding something to the test? I suspect it's redundant in many cases. I'm certainly not reading through it in each diff :)

No I hope not! 😄 I wouldn't expect anyone to do that. I really intended for these to be used in two ways:

If someone changes the MIR in a way that breaks how the CoverageSpans are computed (which could be a big problem), they can compare the differences and resolve the problem.
One can review the CoverageSpans in the spanview rendered format. Reviewing the actual source is way too painful. I hardly ever do that myself, and it's not nearly as helpful as reviewing the rendered form.

I was thinking about inserting a comment at the top of the file with a link to view the file in the browser, using htmlpreview.github.io. I think I'll do that in the next round of fixes based on your comments.

BTW, I was actually also thinking it would be nice to review the new CoverageGraph too. (It would have answered your question about unwind, in a way, for example.)

I could still do that. It would mean adding even more files not intended for review "as source", but the only wan to review the rendered version is (AFAICT) copy it into a local file, or download the patch, and then open it in a graphviz viewer, such as the VSCode plugin I use.

Not sure it's worth it, but it wouldn't be hard to add.

WDYT?

richkadel · 2020-10-25T06:32:06Z

Finally, food for thought: Could some of the things checked by these end-to-end tests actually be checked by unit tests in the code? Unit tests aren't too common in the compiler today, but with as much intricate logic as there is in this code, I think they're worth considering.

Yes, it's a good idea. I've thought about it too. I'm not against it, but the existing tests have worked pretty well so far, so I haven't felt that motivated. Maybe something for the future?

richkadel · 2020-10-25T09:03:44Z

I was thinking about inserting a comment at the top of the file with a link to view the file in the browser, using htmlpreview.github.io. I think I'll do that in the next round of fixes based on your comments.

@tmandry - I'm still working through your feedback, but I wanted to check in a partial response to the first couple of comments; particularly the addition of a link (URL) to a rendered version of the Spanview. (See my comments above.)

See for example:

https://github.com/rust-lang/rust/blob/c3cb945ff97134bf5abac4bedf3a97ffa91a815f/src/test/run-make-fulldeps/coverage-spanview-base/expected_mir_dump.closure/closure.main.-------.InstrumentCoverage.0.html

Select the URL and open it in your browser. It should work for simple function names, but I do have to fix the URL for special characters, and escape things like curly braces. (I just realized that.)

richkadel · 2020-10-25T16:48:03Z

URLs are now escaped:

See the header comment in the following, for example:

https://github.com/rust-lang/rust/blob/64aaaedc56b5d379443c1716224ec20ee95993b3/src/test/run-make-fulldeps/coverage-spanview-base/expected_mir_dump.closure/closure.main-%7Bclosure%230%7D.-------.InstrumentCoverage.0.html

richkadel · 2020-10-25T18:21:55Z

@tmandry Thanks for this first review! I think I've addressed all of the actionable code comments. PTAL.

tmandry · 2020-10-27T22:57:56Z

Thanks for your replies. Regarding panics, I'm not sure what the right thing to do is. But I'm pretty sure there are some cases where we'll want to see correct results in the face of panics, namely #[should_panic] tests, and maybe in code that uses catch_unwind.

Since these are a small percentage of use cases I'm fine with not supporting it just yet, but I think we'll at least want an option to turn on support at some point.

richkadel · 2020-10-27T23:12:26Z

Making it optional is probably the way to go. I will try to file an issue for this, along with the other issue or issues discussed in this set of review comments.

…

On Tue, Oct 27, 2020, 3:58 PM Tyler Mandry ***@***.***> wrote: Thanks for your replies. Regarding panics, I'm not sure what the right thing to do is. But I'm pretty sure there are some cases where we'll want to see correct results in the face of panics, namely #[should_panic] tests, and maybe in code that uses catch_unwind. Since these are a small percentage of use cases I'm fine with not supporting it just yet, but I think we'll at least want an option to turn on support at some point. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#78267 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA5GMYR7LGDST5YUEIE5X7LSM5GAFANCNFSM4S4I3RRA> .

tmandry

Just one commit left to go..

tmandry · 2020-10-27T23:01:12Z

compiler/rustc_mir/src/transform/coverage/counters.rs

@@ -95,6 +105,7 @@ impl CoverageCounters {
    /// Counter IDs start from one and go up.
    fn next_counter(&mut self) -> CounterValueReference {
        assert!(self.next_counter_id < u32::MAX - self.num_expressions);
+        assert!(self.next_counter_id <= MAX_COUNTER_GUARD);


You have + 1 above and <= here. I guess it doesn't really matter but I'd expect one or the other, not both.

Agreed, and at first I did remove the + 1 from the constant def, but (as I will explain below) I believe I'm able to remove MAX_COUNTER_GUARD completely, which is even better. So that second assert! is gone as well.

tmandry · 2020-10-27T23:18:05Z

compiler/rustc_mir/src/transform/coverage/debug.rs

+}
+
+impl DebugOptions {
+    fn new() -> Self {


from_env or similar might be a clearer name than new

tmandry · 2020-10-27T23:20:43Z

compiler/rustc_mir/src/transform/coverage/debug.rs

+        self.some_counters.replace(FxHashMap::default());
+    }
+
+    pub fn is_enabled(&mut self) -> bool {


Doesn't need mut

Agreed. I found a few other functions in debug.rs with the same issue and remove mut in all of them that didn't need it. (Copy-paste issue I think.)

tmandry · 2020-10-27T23:21:03Z

compiler/rustc_mir/src/transform/coverage/debug.rs

+        Self { some_counters: None }
+    }
+
+    pub fn enable(&mut self) {


Might want to assert that it's disabled so you aren't replacing existing data.

Great catch!

I added the following to all enable() functions in debug.rs:

debug_assert!(!self.is_enabled());

richkadel · 2020-10-29T22:27:53Z

I'm pretty sure there are some cases where we'll want to see correct results in the face of panics, namely #[should_panic] tests, and maybe in code that uses catch_unwind.

Since these are a small percentage of use cases I'm fine with not supporting it just yet, but I think we'll at least want an option to turn on support at some point.

I created Issue #78544 for this.

compiler/rustc_mir/src/transform/coverage/debug.rs

richkadel · 2020-10-29T23:14:17Z

Finally, food for thought: Could some of the things checked by these end-to-end tests actually be checked by unit tests in the code? Unit tests aren't too common in the compiler today, but with as much intricate logic as there is in this code, I think they're worth considering.

Yes, it's a good idea. I've thought about it too. I'm not against it, but the existing tests have worked pretty well so far, so I haven't felt that motivated. Maybe something for the future?

Just to capture this idea, I added a short FIXME statement in src/test/run-make-fulldeps/coverage/coverage_tools.mk

tmandry · 2020-10-29T23:23:58Z

compiler/rustc_mir/src/transform/coverage/counters.rs

+        Self { coverage_counters, basic_coverage_blocks }
+    }
+
+    /// If two `CoverageGraph` branch from another `BasicCoverageBlock`, one of the branches


should this say two BasicCoverageBlocks?

Oops! Yes! This happened in a global search/replace a while back. Thanks!

tmandry · 2020-10-29T23:37:16Z

compiler/rustc_mir/src/transform/coverage/counters.rs

+                    bcb,
+                    self.format_counter(counter_kind),
+                );
+                counter_kind.as_operand_id()


suggestion: early return here to reduce rightward drift in the rest of the function

Done. This change in recursive_get_or_make_counter_operand(), and the additional comments (per your other suggestion) make this a bit easier to read and comprehend, I think.

I did the same in recursive_get_or_make_edge_counter_operand() as well.

tmandry · 2020-10-29T23:57:53Z

compiler/rustc_mir/src/transform/coverage/counters.rs

+        // FIXME(richkadel): Add more comments to explain the logic here and in the rest of this
+        // function, and refactor this function to break it up into smaller functions that are
+        // easier to understand.


I think the code is well organized. More comments might help some, but I suspect this file would really benefit from some unit tests that show the cases the logic is intended to handle.

Comments added to help explain the logic, and a FIXME comment to add unit tests, which I'd like to defer to a follow-on (but near term) PR.

tmandry · 2020-10-30T00:01:12Z

compiler/rustc_mir/src/transform/coverage/graph.rs

+            }
+        }
+        let operand = counter_kind.as_operand_id();
+        let expect_none = self


nit: can you use if let here?

Yep. I thought I was making it more readable this way, but after restructuring it again, with the if let this time, it actually seems better. Thanks.

tmandry · 2020-10-30T00:02:52Z

compiler/rustc_mir/src/transform/coverage/graph.rs

+        self.counter_kind.take()
+    }
+
+    #[inline(always)]


should all of these really be #[inline(always)]? some methods seem pretty big for it.

No, my bad. I removed the directive from the larger functions, and removed (always) from a few of them as well.

tmandry · 2020-10-30T00:07:08Z

compiler/rustc_mir/src/transform/coverage/graph.rs

+/// CoverageGraph outside all loops. This supports traversing the BCB CFG in a way that
+/// ensures a loop is completely traversed before processing Blocks after the end of the loop.
+#[derive(Debug)]
+pub(crate) struct TraversalContext {


Another piece of code I think would benefit from some unit tests

I added a FIXME comment to add unit tests here as well.

tmandry · 2020-10-30T00:17:16Z

compiler/rustc_mir/src/transform/coverage/graph.rs

+    let mut backedges = IndexVec::from_elem_n(Vec::<BasicCoverageBlock>::new(), num_bcbs);
+
+    // Identify loops by their backedges
+    for (bcb, _) in basic_coverage_blocks.iter_enumerated() {


What's the complexity of this? Thinking about how it would handle very large functions

Since it evoked your question, I added a comment in the code for this. I took a bit of an educated guess at the big-O complexity equation (not something I feel super comfortable with), so please read my comment and let me know if you would state it differently.

Bottom line, I think the complexity is something close to O(n log n), which I think is acceptable (and not particularly unique compared to other parts of the compiler, unless I'm missing something).

Does the explanation and conclusion look right to you?

This breakdown helps, thanks. I don't want to prematurely optimize but it's nice to have a mental model for thinking about later if it is a problem.

If it were O(n log n) the number of dominators per node would have to be O(log n) which doesn't seem obviously right to me.

(The simplest counterexample – a long function with no branching – would be one long BCB, which highlights the benefits of simplifying the graph into BCBs. But I suspect one could possibly extend that idea to construct a graph whose dominators grew as a linear factor of the length of the function.)

Maybe it's easiest to leave that as a separate parameter?

tmandry · 2020-10-30T00:18:53Z

compiler/rustc_mir/src/transform/coverage/mod.rs

+                ////////////////////////////////////////////////////
+                // Remove the counter or edge counter from of each `CoverageSpan`s associated
+                // `BasicCoverageBlock`, and inject a `Coverage` statement into the MIR.
+                self.inject_coverage_span_counters(
+                    coverage_spans,
+                    &mut graphviz_data,
+                    &mut debug_used_expressions,
+                );
+
+                ////////////////////////////////////////////////////
+                // For any remaining `BasicCoverageBlock` counters (that were not associated with
+                // any `CoverageSpan`), inject `Coverage` statements (_without_ code region `Span`s)
+                // to ensure `BasicCoverageBlock` counters that other `Expression`s may depend on
+                // are in fact counted, even though they don't directly contribute to counting
+                // their own independent code region's coverage.
+                self.inject_indirect_counters(&mut graphviz_data, &mut debug_used_expressions);


Can you say more about why things are injected in this particular order?

More comments added to explain this. Does this work?

richkadel · 2020-10-30T21:57:58Z

@tmandry @wesleywiser - I think I've addressed all of the remaining feedback, but I'm re-running tests locally, first, before I upload the latest changes.

I may respond to some of the review feedback before the changes are uploaded, but they changes should be up in less than an hour, I expect.

Tyler - I added some FIXME comments to add unit tests, as you suggested. Are you OK if I defer the unit tests to a follow-on (near term) PR?

richkadel · 2020-10-30T23:11:05Z

@tmandry @wesleywiser - the changes are uploaded

tmandry

Tyler - I added some FIXME comments to add unit tests, as you suggested. Are you OK if I defer the unit tests to a follow-on (near term) PR?

Yeah that's fine.

LGTM - @wesleywiser do you want to review more or should we r+?

tmandry · 2020-10-31T00:34:55Z

compiler/rustc_mir/src/transform/coverage/graph.rs

+    let mut backedges = IndexVec::from_elem_n(Vec::<BasicCoverageBlock>::new(), num_bcbs);
+
+    // Identify loops by their backedges
+    for (bcb, _) in basic_coverage_blocks.iter_enumerated() {


This breakdown helps, thanks. I don't want to prematurely optimize but it's nice to have a mental model for thinking about later if it is a problem.

If it were O(n log n) the number of dominators per node would have to be O(log n) which doesn't seem obviously right to me.

(The simplest counterexample – a long function with no branching – would be one long BCB, which highlights the benefits of simplifying the graph into BCBs. But I suspect one could possibly extend that idea to construct a graph whose dominators grew as a linear factor of the length of the function.)

Maybe it's easiest to leave that as a separate parameter?

wesleywiser · 2020-10-31T02:42:59Z

@tmandry I looked though briefly but I don't think I'll have time to review this in detail for at least a few days. If you think this is good to go, don't let me hold you up!

bors · 2020-11-01T19:41:30Z

☔ The latest upstream changes (presumably #75534) 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-06T02:28:07Z

The sed command breaks on MacOS, and the simple fix will likely produce inconsistent results. I'm disabling this on MacOS for now.

richkadel · 2020-11-06T02:28:55Z

@bors retry r=tmandry

bors · 2020-11-06T02:28:57Z

📌 Commit ece2792f1dab759706a2f04c75aeeb8ad2deb5ac has been approved by tmandry

richkadel · 2020-11-06T04:19:25Z

@bors retry r=tmandry

bors · 2020-11-06T04:19:27Z

📌 Commit bf0f0425ad73b27b84ae549cd0a2ece1e9769510 has been approved by tmandry

And even though CI should now pass for MacOS, the llvm-cov show --debug flag does not work when developing outside of CI, so I'm disabling it for MacOS by default.

richkadel · 2020-11-06T04:25:51Z

@bors r=tmandry

bors · 2020-11-06T04:25:52Z

📌 Commit 68014e6 has been approved by tmandry

richkadel · 2020-11-06T04:27:38Z

I found a typo in my MacOS fix and resolved it, and then found that --debug in llvm-cov show doesn't work even when developing outside of CI. (The CI should pass now, but without disabling it for all MacOS builds, it would have failed for developers.) Took care of that as well.

bors · 2020-11-06T06:59:50Z

⌛ Testing commit 68014e6 with merge 8532e74...

bors · 2020-11-06T09:18:51Z

☀️ Test successful - checks-actions
Approved by: tmandry
Pushing 8532e74 to master...

As discussed in PR rust-lang#78267, for example: * rust-lang#78267 (comment) * rust-lang#78267 (comment)

…0.4, r=tmandry Added some unit tests as requested As discussed in PR rust-lang#78267, for example: * rust-lang#78267 (comment) * rust-lang#78267 (comment) r? `@tmandry` FYI: `@wesleywiser` This is pretty much self contained, but depending on feedback and timing, I may have a chance to add a few more unit tests requested against `counters.rs`. I'm looking at those now.

…0.4, r=tmandry Added some unit tests as requested As discussed in PR rust-lang#78267, for example: * rust-lang#78267 (comment) * rust-lang#78267 (comment) r? ``@tmandry`` FYI: ``@wesleywiser`` This is pretty much self contained, but depending on feedback and timing, I may have a chance to add a few more unit tests requested against `counters.rs`. I'm looking at those now.

…0.4, r=tmandry Added some unit tests as requested As discussed in PR rust-lang#78267, for example: * rust-lang#78267 (comment) * rust-lang#78267 (comment) r? ```@tmandry``` FYI: ```@wesleywiser``` This is pretty much self contained, but depending on feedback and timing, I may have a chance to add a few more unit tests requested against `counters.rs`. I'm looking at those now.

…0.4, r=tmandry Added some unit tests as requested As discussed in PR rust-lang#78267, for example: * rust-lang#78267 (comment) * rust-lang#78267 (comment) r? ```````@tmandry``````` FYI: ```````@wesleywiser``````` This is pretty much self contained, but depending on feedback and timing, I may have a chance to add a few more unit tests requested against `counters.rs`. I'm looking at those now.

rust-highfive assigned tmandry Oct 23, 2020

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

richkadel mentioned this pull request Oct 23, 2020

[WIP] Working expression optimization, and some improvements to branch-level source coverage #78040

Closed

richkadel force-pushed the llvm-coverage-counters-2.0.3r1 branch from d321d63 to 3cf8342 Compare October 23, 2020 09:04

richkadel force-pushed the llvm-coverage-counters-2.0.3r1 branch from 3cf8342 to 351e004 Compare October 24, 2020 22:08

tmandry reviewed Oct 24, 2020

View reviewed changes

richkadel force-pushed the llvm-coverage-counters-2.0.3r1 branch from c3cb945 to 64aaaed Compare October 25, 2020 16:42

richkadel force-pushed the llvm-coverage-counters-2.0.3r1 branch from 64aaaed to a5e087c Compare October 25, 2020 16:54

tmandry reviewed Oct 27, 2020

View reviewed changes

wesleywiser reviewed Oct 29, 2020

View reviewed changes

compiler/rustc_mir/src/transform/coverage/debug.rs Show resolved Hide resolved

tmandry reviewed Oct 30, 2020

View reviewed changes

richkadel force-pushed the llvm-coverage-counters-2.0.3r1 branch from af58227 to a695c3f Compare October 30, 2020 23:46

tmandry approved these changes Oct 31, 2020

View reviewed changes

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 Nov 6, 2020

richkadel force-pushed the llvm-coverage-counters-2.0.3r1 branch from ece2792 to bf0f042 Compare November 6, 2020 04:19

Disable the spanview tests on MacOS for now

68014e6

And even though CI should now pass for MacOS, the llvm-cov show --debug flag does not work when developing outside of CI, so I'm disabling it for MacOS by default.

richkadel force-pushed the llvm-coverage-counters-2.0.3r1 branch from bf0f042 to 68014e6 Compare November 6, 2020 04:25

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 6, 2020

bors merged commit 8532e74 into rust-lang:master Nov 6, 2020

rustbot added this to the 1.49.0 milestone Nov 6, 2020

This was referenced Nov 6, 2020

Add StatementKind::CopyNonOverlapping #77511

Merged

make MIR graphviz generation use gsgdt #78399

Merged

This was referenced Nov 6, 2020

Implement LLVM-compatible source-based code coverage rust-lang/compiler-team#278

Closed

Implement support for LLVMs code coverage instrumentation #34701

Closed

richkadel mentioned this pull request Nov 12, 2020

Added some unit tests as requested #78963

Merged

richkadel added a commit to richkadel/rust that referenced this pull request Nov 12, 2020

Added some unit tests as requested

bd0eb07

As discussed in PR rust-lang#78267, for example: * rust-lang#78267 (comment) * rust-lang#78267 (comment)

richkadel mentioned this pull request Nov 17, 2020

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

Closed

3 tasks

		\| TerminatorKind::FalseEdge { .. } => None,

		// FIXME(richkadel): Note that `Goto` was initially filtered out (by returning `None`, as

Working expression optimization, and some improvements to branch-level source coverage #78267

Working expression optimization, and some improvements to branch-level source coverage #78267

Conversation

richkadel commented Oct 23, 2020

richkadel commented Oct 23, 2020

bors commented Oct 24, 2020

richkadel commented Oct 24, 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

richkadel commented Oct 25, 2020

richkadel commented Oct 25, 2020

richkadel commented Oct 25, 2020

richkadel commented Oct 25, 2020

richkadel commented Oct 25, 2020

richkadel commented Oct 25, 2020

tmandry commented Oct 27, 2020

richkadel commented Oct 27, 2020 via email

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

richkadel Oct 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richkadel commented Oct 29, 2020

richkadel commented Oct 29, 2020

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 Oct 30, 2020

richkadel commented Oct 30, 2020

tmandry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesleywiser commented Oct 31, 2020

bors commented Nov 1, 2020

richkadel commented Nov 6, 2020

richkadel commented Nov 6, 2020

bors commented Nov 6, 2020

richkadel commented Nov 6, 2020

bors commented Nov 6, 2020

richkadel commented Nov 6, 2020

bors commented Nov 6, 2020

richkadel commented Nov 6, 2020

bors commented Nov 6, 2020

bors commented Nov 6, 2020

richkadel Oct 30, 2020 •

edited

Loading