Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed conflict with drop elaboration and coverage #80072

Merged
merged 1 commit into from
Dec 17, 2020
Merged

Fixed conflict with drop elaboration and coverage #80072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions compiler/rustc_mir/src/transform/coverage/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
inject_statement(
self.mir_body,
counter_kind,
self.bcb_last_bb(bcb),
self.bcb_leader_bb(bcb),
Some(make_code_region(file_name, &self.source_file, span, body_span)),
);
}
Expand Down Expand Up @@ -470,7 +470,7 @@ fn inject_statement(
code_region: some_code_region,
}),
};
data.statements.push(statement);
data.statements.insert(0, statement);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is always going to be slower than appending at the end. I'd prefer to skip over coverage statements in the elaborate drops pass rather than work around it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually prefer it this way and I think the overall impact will be negligible.

Yes, functionally, we have the option of inserting the statement anywhere in the BCB, but it is standard practice to insert() into the statements vec in MIR processing when order is critical.

As I was trying to say in my reply here (though maybe not clearly):

#80045 (comment)

I believe inserting the coverage statement between the last statement and the terminator is going to break up some sequences in unexpected ways.

It wouldn't be a problem if I could insert the coverage statement after the terminator (which is impossible without a new BB, and that's much worse).

But the Terminator is really just a special case of a Statement, and the sequence from one statement to another is sometimes important, analytically at least. (Possibly even important at runtime, for context switching reasons, in fact.)

To drive the point: Think about a typical instruction sequence like, Load followed by Add (adding to the value that was just loaded). Most people would expect these to come in uninterrupted sequence, so injecting code to increment a global counter between the two, even if possible, would not be right.

I would rather not insert Coverage statements between a Statement and it's following Terminator, and it's not just about this one issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed your comment there, makes sense.

}

// Non-code expressions are injected into the coverage map, without generating executable code.
Expand Down
2 changes: 1 addition & 1 deletion src/test/mir-opt/coverage_graphviz.bar.InstrumentCoverage.0.dot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@ digraph Cov_0_4 {
graph [fontname="Courier, monospace"];
node [fontname="Courier, monospace"];
edge [fontname="Courier, monospace"];
bcb0__Cov_0_4 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb0</td></tr><tr><td align="left" balign="left"></td></tr><tr><td align="left" balign="left">Counter(bcb0) at 18:1-20:2<br/> 19:5-19:9: @0[0]: _0 = const true<br/> 20:2-20:2: @0.Return: return</td></tr><tr><td align="left" balign="left">bb0: Return</td></tr></table>>];
bcb0__Cov_0_4 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb0</td></tr><tr><td align="left" balign="left"></td></tr><tr><td align="left" balign="left">Counter(bcb0) at 18:1-20:2<br/> 19:5-19:9: @0[0]: Coverage::Counter(1) for $DIR/coverage_graphviz.rs:18:1 - 20:2<br/> 20:2-20:2: @0.Return: return</td></tr><tr><td align="left" balign="left">bb0: Return</td></tr></table>>];
}
2 changes: 1 addition & 1 deletion src/test/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ digraph Cov_0_3 {
graph [fontname="Courier, monospace"];
node [fontname="Courier, monospace"];
edge [fontname="Courier, monospace"];
bcb2__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb2</td></tr><tr><td align="left" balign="left">Expression(bcb0 - bcb1) at 13:10-13:10<br/> 13:10-13:10: @4[0]: _1 = const ()</td></tr><tr><td align="left" balign="left">bb4: Goto</td></tr></table>>];
bcb2__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb2</td></tr><tr><td align="left" balign="left">Expression(bcb0 - bcb1) at 13:10-13:10<br/> 13:10-13:10: @4[0]: Coverage::Expression(4294967295) = 1 - 2 for $DIR/coverage_graphviz.rs:13:10 - 13:11</td></tr><tr><td align="left" balign="left">bb4: Goto</td></tr></table>>];
tmandry marked this conversation as resolved.
Show resolved Hide resolved
bcb1__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb1</td></tr><tr><td align="left" balign="left">Counter(bcb1) at 12:13-12:18<br/> 12:13-12:18: @5[0]: _0 = const ()<br/>Expression(bcb1 + 0) at 15:2-15:2<br/> 15:2-15:2: @5.Return: return</td></tr><tr><td align="left" balign="left">bb3: FalseEdge</td></tr><tr><td align="left" balign="left">bb5: Return</td></tr></table>>];
bcb0__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb0</td></tr><tr><td align="left" balign="left"></td></tr><tr><td align="left" balign="left">Counter(bcb0) at 9:1-11:17<br/> 11:12-11:17: @1.Call: _2 = bar() -&gt; [return: bb2, unwind: bb6]<br/> 11:12-11:17: @2[0]: FakeRead(ForMatchedPlace, _2)</td></tr><tr><td align="left" balign="left">bb0: FalseUnwind<br/>bb1: Call</td></tr><tr><td align="left" balign="left">bb2: SwitchInt</td></tr></table>>];
bcb2__Cov_0_3 -> bcb0__Cov_0_3 [label=<>];
Expand Down
2 changes: 1 addition & 1 deletion src/test/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
let mut _0: bool; // return place in scope 0 at /the/src/instrument_coverage.rs:19:13: 19:17

bb0: {
_0 = const true; // scope 0 at /the/src/instrument_coverage.rs:20:5: 20:9
+ Coverage::Counter(1) for /the/src/instrument_coverage.rs:19:1 - 21:2; // scope 0 at /the/src/instrument_coverage.rs:21:2: 21:2
_0 = const true; // scope 0 at /the/src/instrument_coverage.rs:20:5: 20:9
return; // scope 0 at /the/src/instrument_coverage.rs:21:2: 21:2
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/test/mir-opt/instrument_coverage.main.InstrumentCoverage.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
let mut _3: !; // in scope 0 at /the/src/instrument_coverage.rs:12:18: 14:10

bb0: {
+ Coverage::Counter(1) for /the/src/instrument_coverage.rs:10:1 - 12:17; // scope 0 at /the/src/instrument_coverage.rs:11:5: 15:6
falseUnwind -> [real: bb1, cleanup: bb6]; // scope 0 at /the/src/instrument_coverage.rs:11:5: 15:6
}

Expand All @@ -21,26 +22,25 @@

bb2: {
FakeRead(ForMatchedPlace, _2); // scope 0 at /the/src/instrument_coverage.rs:12:12: 12:17
+ Coverage::Counter(1) for /the/src/instrument_coverage.rs:10:1 - 12:17; // scope 0 at /the/src/instrument_coverage.rs:12:9: 14:10
switchInt(_2) -> [false: bb4, otherwise: bb3]; // scope 0 at /the/src/instrument_coverage.rs:12:9: 14:10
}

bb3: {
+ Coverage::Expression(4294967294) = 2 + 0 for /the/src/instrument_coverage.rs:16:1 - 16:2; // scope 0 at /the/src/instrument_coverage.rs:12:9: 14:10
+ Coverage::Counter(2) for /the/src/instrument_coverage.rs:13:13 - 13:18; // scope 0 at /the/src/instrument_coverage.rs:12:9: 14:10
falseEdge -> [real: bb5, imaginary: bb4]; // scope 0 at /the/src/instrument_coverage.rs:12:9: 14:10
}

bb4: {
+ Coverage::Expression(4294967295) = 1 - 2 for /the/src/instrument_coverage.rs:14:10 - 14:11; // scope 0 at /the/src/instrument_coverage.rs:11:5: 15:6
_1 = const (); // scope 0 at /the/src/instrument_coverage.rs:14:10: 14:10
StorageDead(_2); // scope 0 at /the/src/instrument_coverage.rs:15:5: 15:6
+ Coverage::Expression(4294967295) = 1 - 2 for /the/src/instrument_coverage.rs:14:10 - 14:11; // scope 0 at /the/src/instrument_coverage.rs:11:5: 15:6
goto -> bb0; // scope 0 at /the/src/instrument_coverage.rs:11:5: 15:6
}

bb5: {
_0 = const (); // scope 0 at /the/src/instrument_coverage.rs:13:13: 13:18
StorageDead(_2); // scope 0 at /the/src/instrument_coverage.rs:15:5: 15:6
+ Coverage::Counter(2) for /the/src/instrument_coverage.rs:13:13 - 13:18; // scope 0 at /the/src/instrument_coverage.rs:16:2: 16:2
+ Coverage::Expression(4294967294) = 2 + 0 for /the/src/instrument_coverage.rs:16:1 - 16:2; // scope 0 at /the/src/instrument_coverage.rs:16:2: 16:2
return; // scope 0 at /the/src/instrument_coverage.rs:16:2: 16:2
}

Expand Down
20 changes: 10 additions & 10 deletions src/test/run-make-fulldeps/coverage-reports/expected_export_coverage.assert.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@
},
"lines": {
"count": 15,
"covered": 12,
"percent": 80
"covered": 13,
"percent": 86.66666666666667
},
"regions": {
"count": 14,
"covered": 12,
"notcovered": 2,
"percent": 85.71428571428571
"covered": 13,
"notcovered": 1,
"percent": 92.85714285714286
}
}
}
Expand All @@ -42,14 +42,14 @@
},
"lines": {
"count": 15,
"covered": 12,
"percent": 80
"covered": 13,
"percent": 86.66666666666667
},
"regions": {
"count": 14,
"covered": 12,
"notcovered": 2,
"percent": 85.71428571428571
"covered": 13,
"notcovered": 1,
"percent": 92.85714285714286
}
}
}
Expand Down
20 changes: 10 additions & 10 deletions src/test/run-make-fulldeps/coverage-reports/expected_export_coverage.overflow.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@
},
"lines": {
"count": 23,
"covered": 19,
"percent": 82.6086956521739
"covered": 21,
"percent": 91.30434782608695
},
"regions": {
"count": 13,
"covered": 11,
"notcovered": 2,
"percent": 84.61538461538461
"covered": 12,
"notcovered": 1,
"percent": 92.3076923076923
}
}
}
Expand All @@ -42,14 +42,14 @@
},
"lines": {
"count": 23,
"covered": 19,
"percent": 82.6086956521739
"covered": 21,
"percent": 91.30434782608695
},
"regions": {
"count": 13,
"covered": 11,
"notcovered": 2,
"percent": 84.61538461538461
"covered": 12,
"notcovered": 1,
"percent": 92.3076923076923
}
}
}
Expand Down
20 changes: 10 additions & 10 deletions src/test/run-make-fulldeps/coverage-reports/expected_export_coverage.panic_unwind.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@
},
"lines": {
"count": 19,
"covered": 16,
"percent": 84.21052631578947
"covered": 17,
"percent": 89.47368421052632
},
"regions": {
"count": 13,
"covered": 11,
"notcovered": 2,
"percent": 84.61538461538461
"covered": 12,
"notcovered": 1,
"percent": 92.3076923076923
}
}
}
Expand All @@ -42,14 +42,14 @@
},
"lines": {
"count": 19,
"covered": 16,
"percent": 84.21052631578947
"covered": 17,
"percent": 89.47368421052632
},
"regions": {
"count": 13,
"covered": 11,
"notcovered": 2,
"percent": 84.61538461538461
"covered": 12,
"notcovered": 1,
"percent": 92.3076923076923
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.assert.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@
8| |
9| 1|fn main() -> Result<(),u8> {
10| 1| let mut countdown = 10;
11| 10| while countdown > 0 {
12| 10| if countdown == 1 {
13| 0| might_fail_assert(3);
11| 11| while countdown > 0 {
12| 11| if countdown == 1 {
13| 1| might_fail_assert(3);
14| 10| } else if countdown < 5 {
15| 3| might_fail_assert(2);
16| 6| }
17| 9| countdown -= 1;
17| 10| countdown -= 1;
18| | }
19| 0| Ok(())
20| 0|}
Expand Down
10 changes: 5 additions & 5 deletions src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.overflow.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,15 @@
14| |
15| 1|fn main() -> Result<(),u8> {
16| 1| let mut countdown = 10;
17| 10| while countdown > 0 {
18| 10| if countdown == 1 {
19| 0| let result = might_overflow(10);
20| 0| println!("Result: {}", result);
17| 11| while countdown > 0 {
18| 11| if countdown == 1 {
19| 1| let result = might_overflow(10);
20| 1| println!("Result: {}", result);
21| 10| } else if countdown < 5 {
22| 3| let result = might_overflow(1);
23| 3| println!("Result: {}", result);
24| 6| }
25| 9| countdown -= 1;
25| 10| countdown -= 1;
26| | }
27| 0| Ok(())
28| 0|}
Expand Down
8 changes: 4 additions & 4 deletions src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.panic_unwind.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@
12| |
13| 1|fn main() -> Result<(), u8> {
14| 1| let mut countdown = 10;
15| 10| while countdown > 0 {
16| 10| if countdown == 1 {
17| 0| might_panic(true);
15| 11| while countdown > 0 {
16| 11| if countdown == 1 {
17| 1| might_panic(true);
18| 10| } else if countdown < 5 {
19| 3| might_panic(false);
20| 6| }
21| 9| countdown -= 1;
21| 10| countdown -= 1;
22| | }
23| 0| Ok(())
24| 0|}
Expand Down
16 changes: 8 additions & 8 deletions src/test/run-make-fulldeps/coverage-reports/expected_show_coverage_counters.assert.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,13 @@ Combined regions:
6:37 -> 6:61 (count=1)
7:1 -> 7:2 (count=3)
9:1 -> 10:27 (count=1)
11:11 -> 11:24 (count=10)
12:12 -> 12:26 (count=10)
12:27 -> 14:10 (count=0)
11:11 -> 11:24 (count=11)
12:12 -> 12:26 (count=11)
12:27 -> 14:10 (count=1)
14:19 -> 14:32 (count=10)
14:33 -> 16:10 (count=3)
16:10 -> 16:11 (count=6)
17:9 -> 17:23 (count=9)
17:9 -> 17:23 (count=10)
19:5 -> 20:2 (count=0)
Segment at 4:1 (count = 4), RegionEntry
Segment at 4:41 (count = 0), Skipped
Expand All @@ -40,18 +40,18 @@ Segment at 7:1 (count = 3), RegionEntry
Segment at 7:2 (count = 0), Skipped
Segment at 9:1 (count = 1), RegionEntry
Segment at 10:27 (count = 0), Skipped
Segment at 11:11 (count = 10), RegionEntry
Segment at 11:11 (count = 11), RegionEntry
Segment at 11:24 (count = 0), Skipped
Segment at 12:12 (count = 10), RegionEntry
Segment at 12:12 (count = 11), RegionEntry
Segment at 12:26 (count = 0), Skipped
Segment at 12:27 (count = 0), RegionEntry
Segment at 12:27 (count = 1), RegionEntry
Segment at 14:10 (count = 0), Skipped
Segment at 14:19 (count = 10), RegionEntry
Segment at 14:32 (count = 0), Skipped
Segment at 14:33 (count = 3), RegionEntry
Segment at 16:10 (count = 6), RegionEntry
Segment at 16:11 (count = 0), Skipped
Segment at 17:9 (count = 9), RegionEntry
Segment at 17:9 (count = 10), RegionEntry
Segment at 17:23 (count = 0), Skipped
Segment at 19:5 (count = 0), RegionEntry
Segment at 20:2 (count = 0), Skipped
16 changes: 8 additions & 8 deletions src/test/run-make-fulldeps/coverage-reports/expected_show_coverage_counters.overflow.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@ Combined regions:
7:6 -> 7:7 (count=3)
8:9 -> 13:2 (count=4)
15:1 -> 16:27 (count=1)
17:11 -> 17:24 (count=10)
18:12 -> 18:26 (count=10)
18:27 -> 21:10 (count=0)
17:11 -> 17:24 (count=11)
18:12 -> 18:26 (count=11)
18:27 -> 21:10 (count=1)
21:19 -> 21:32 (count=10)
21:33 -> 24:10 (count=3)
24:10 -> 24:11 (count=6)
25:9 -> 25:23 (count=9)
25:9 -> 25:23 (count=10)
27:5 -> 28:2 (count=0)
Segment at 4:1 (count = 4), RegionEntry
Segment at 5:18 (count = 0), Skipped
Expand All @@ -35,18 +35,18 @@ Segment at 8:9 (count = 4), RegionEntry
Segment at 13:2 (count = 0), Skipped
Segment at 15:1 (count = 1), RegionEntry
Segment at 16:27 (count = 0), Skipped
Segment at 17:11 (count = 10), RegionEntry
Segment at 17:11 (count = 11), RegionEntry
Segment at 17:24 (count = 0), Skipped
Segment at 18:12 (count = 10), RegionEntry
Segment at 18:12 (count = 11), RegionEntry
Segment at 18:26 (count = 0), Skipped
Segment at 18:27 (count = 0), RegionEntry
Segment at 18:27 (count = 1), RegionEntry
Segment at 21:10 (count = 0), Skipped
Segment at 21:19 (count = 10), RegionEntry
Segment at 21:32 (count = 0), Skipped
Segment at 21:33 (count = 3), RegionEntry
Segment at 24:10 (count = 6), RegionEntry
Segment at 24:11 (count = 0), Skipped
Segment at 25:9 (count = 9), RegionEntry
Segment at 25:9 (count = 10), RegionEntry
Segment at 25:23 (count = 0), Skipped
Segment at 27:5 (count = 0), RegionEntry
Segment at 28:2 (count = 0), Skipped
16 changes: 8 additions & 8 deletions src/test/run-make-fulldeps/coverage-reports/expected_show_coverage_counters.panic_unwind.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@ Combined regions:
6:9 -> 7:26 (count=1)
8:12 -> 11:2 (count=3)
13:1 -> 14:27 (count=1)
15:11 -> 15:24 (count=10)
16:12 -> 16:26 (count=10)
16:27 -> 18:10 (count=0)
15:11 -> 15:24 (count=11)
16:12 -> 16:26 (count=11)
16:27 -> 18:10 (count=1)
18:19 -> 18:32 (count=10)
18:33 -> 20:10 (count=3)
20:10 -> 20:11 (count=6)
21:9 -> 21:23 (count=9)
21:9 -> 21:23 (count=10)
23:5 -> 24:2 (count=0)
Segment at 4:1 (count = 4), RegionEntry
Segment at 4:36 (count = 0), Skipped
Expand All @@ -36,18 +36,18 @@ Segment at 8:12 (count = 3), RegionEntry
Segment at 11:2 (count = 0), Skipped
Segment at 13:1 (count = 1), RegionEntry
Segment at 14:27 (count = 0), Skipped
Segment at 15:11 (count = 10), RegionEntry
Segment at 15:11 (count = 11), RegionEntry
Segment at 15:24 (count = 0), Skipped
Segment at 16:12 (count = 10), RegionEntry
Segment at 16:12 (count = 11), RegionEntry
Segment at 16:26 (count = 0), Skipped
Segment at 16:27 (count = 0), RegionEntry
Segment at 16:27 (count = 1), RegionEntry
Segment at 18:10 (count = 0), Skipped
Segment at 18:19 (count = 10), RegionEntry
Segment at 18:32 (count = 0), Skipped
Segment at 18:33 (count = 3), RegionEntry
Segment at 20:10 (count = 6), RegionEntry
Segment at 20:11 (count = 0), Skipped
Segment at 21:9 (count = 9), RegionEntry
Segment at 21:9 (count = 10), RegionEntry
Segment at 21:23 (count = 0), Skipped
Segment at 23:5 (count = 0), RegionEntry
Segment at 24:2 (count = 0), Skipped