From 4690f97099c2f01a07a8fa272e94e3e5cba61f12 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Mon, 18 Sep 2023 21:11:14 +1000 Subject: [PATCH] coverage: Fix an unstable-sort inconsistency in coverage spans This code was calling `sort_unstable_by`, but failed to impose a total order on the initial spans. That resulted in unpredictable handling of closure spans, producing inconsistencies in the coverage maps and in user-visible coverage reports. This patch fixes the problem by always sorting closure spans before otherwise-identical non-closure spans, and also switches to a stable sort in case the ordering is still not total. --- compiler/rustc_mir_transform/src/coverage/spans.rs | 5 ++++- tests/coverage-map/status-quo/closure.cov-map | 14 +++++++------- tests/coverage-map/status-quo/generator.cov-map | 4 ++-- tests/run-coverage/closure.coverage | 14 +++++++------- 4 files changed, 20 insertions(+), 17 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/spans.rs b/compiler/rustc_mir_transform/src/coverage/spans.rs index 717763a94a0..d254c1662e4 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans.rs @@ -333,7 +333,7 @@ impl<'a, 'tcx> CoverageSpans<'a, 'tcx> { initial_spans.push(CoverageSpan::for_fn_sig(self.fn_sig_span)); - initial_spans.sort_unstable_by(|a, b| { + initial_spans.sort_by(|a, b| { if a.span.lo() == b.span.lo() { if a.span.hi() == b.span.hi() { if a.is_in_same_bcb(b) { @@ -357,6 +357,9 @@ impl<'a, 'tcx> CoverageSpans<'a, 'tcx> { a.span.lo().partial_cmp(&b.span.lo()) } .unwrap() + // If two spans are otherwise identical, put closure spans first, + // as this seems to be what the refinement step expects. + .then_with(|| Ord::cmp(&a.is_closure, &b.is_closure).reverse()) }); initial_spans diff --git a/tests/coverage-map/status-quo/closure.cov-map b/tests/coverage-map/status-quo/closure.cov-map index f3a32f091a9..7dbf6ec834d 100644 --- a/tests/coverage-map/status-quo/closure.cov-map +++ b/tests/coverage-map/status-quo/closure.cov-map @@ -1,5 +1,5 @@ Function name: closure::main -Raw bytes (170): 0x[01, 01, 17, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 05, 05, 5a, 01, 05, 18, 01, 08, 01, 0f, 0d, 03, 16, 0e, 06, 0a, 07, 10, 05, 13, 0d, 0b, 1a, 0e, 08, 09, 0f, 10, 05, 0e, 09, 13, 16, 05, 0d, 18, 17, 19, 09, 01, 21, 1b, 04, 09, 00, 29, 1f, 01, 09, 00, 2d, 23, 01, 09, 00, 24, 27, 05, 09, 00, 24, 2b, 02, 09, 00, 21, 2f, 04, 09, 00, 21, 33, 04, 09, 00, 28, 37, 09, 09, 00, 32, 3b, 04, 09, 00, 33, 3f, 07, 09, 00, 4b, 43, 08, 09, 01, 09, 47, 0a, 09, 01, 09, 4b, 08, 09, 01, 09, 4f, 0a, 08, 00, 10, 05, 00, 11, 04, 06, 5a, 04, 06, 00, 07, 57, 01, 05, 03, 02] +Raw bytes (170): 0x[01, 01, 17, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 00, 01, 05, 05, 5a, 01, 05, 18, 01, 08, 01, 0f, 0d, 03, 16, 0e, 06, 0a, 07, 10, 05, 13, 0d, 0b, 1a, 0e, 06, 0a, 0f, 10, 05, 0c, 16, 13, 16, 05, 0d, 18, 17, 19, 09, 01, 1e, 1b, 04, 09, 00, 29, 1f, 01, 09, 00, 2d, 23, 01, 09, 00, 24, 27, 05, 09, 00, 24, 2b, 02, 09, 00, 21, 2f, 04, 09, 00, 21, 33, 04, 09, 00, 28, 37, 09, 09, 00, 32, 3b, 04, 09, 00, 33, 3f, 07, 09, 00, 4b, 43, 08, 09, 00, 48, 47, 0a, 09, 00, 47, 4b, 08, 09, 00, 44, 4f, 0a, 08, 00, 10, 05, 00, 11, 04, 06, 5a, 04, 06, 00, 07, 57, 01, 05, 03, 02] Number of files: 1 - file 0 => global file 1 Number of expressions: 23 @@ -32,13 +32,13 @@ Number of file 0 mappings: 24 = (c0 + Zero) - Code(Expression(1, Add)) at (prev + 16, 5) to (start + 19, 13) = (c0 + Zero) -- Code(Expression(2, Add)) at (prev + 26, 14) to (start + 8, 9) +- Code(Expression(2, Add)) at (prev + 26, 14) to (start + 6, 10) = (c0 + Zero) -- Code(Expression(3, Add)) at (prev + 16, 5) to (start + 14, 9) +- Code(Expression(3, Add)) at (prev + 16, 5) to (start + 12, 22) = (c0 + Zero) - Code(Expression(4, Add)) at (prev + 22, 5) to (start + 13, 24) = (c0 + Zero) -- Code(Expression(5, Add)) at (prev + 25, 9) to (start + 1, 33) +- Code(Expression(5, Add)) at (prev + 25, 9) to (start + 1, 30) = (c0 + Zero) - Code(Expression(6, Add)) at (prev + 4, 9) to (start + 0, 41) = (c0 + Zero) @@ -60,11 +60,11 @@ Number of file 0 mappings: 24 = (c0 + Zero) - Code(Expression(15, Add)) at (prev + 7, 9) to (start + 0, 75) = (c0 + Zero) -- Code(Expression(16, Add)) at (prev + 8, 9) to (start + 1, 9) +- Code(Expression(16, Add)) at (prev + 8, 9) to (start + 0, 72) = (c0 + Zero) -- Code(Expression(17, Add)) at (prev + 10, 9) to (start + 1, 9) +- Code(Expression(17, Add)) at (prev + 10, 9) to (start + 0, 71) = (c0 + Zero) -- Code(Expression(18, Add)) at (prev + 8, 9) to (start + 1, 9) +- Code(Expression(18, Add)) at (prev + 8, 9) to (start + 0, 68) = (c0 + Zero) - Code(Expression(19, Add)) at (prev + 10, 8) to (start + 0, 16) = (c0 + Zero) diff --git a/tests/coverage-map/status-quo/generator.cov-map b/tests/coverage-map/status-quo/generator.cov-map index 6e10b58a941..a66c1af30f9 100644 --- a/tests/coverage-map/status-quo/generator.cov-map +++ b/tests/coverage-map/status-quo/generator.cov-map @@ -14,7 +14,7 @@ Number of file 0 mappings: 4 = (c1 + (c0 - c1)) Function name: generator::main -Raw bytes (71): 0x[01, 01, 0b, 01, 00, 05, 0b, 09, 0d, 11, 00, 11, 15, 2a, 19, 11, 15, 15, 19, 26, 00, 2a, 19, 11, 15, 09, 01, 0f, 01, 02, 19, 03, 07, 0b, 00, 2e, 11, 01, 2b, 00, 2d, 07, 01, 0e, 00, 35, 0f, 02, 0b, 00, 2e, 2a, 01, 22, 00, 27, 26, 00, 2c, 00, 2e, 1f, 01, 0e, 00, 35, 23, 02, 01, 00, 02] +Raw bytes (71): 0x[01, 01, 0b, 01, 00, 05, 0b, 09, 0d, 11, 00, 11, 15, 2a, 19, 11, 15, 15, 19, 26, 00, 2a, 19, 11, 15, 09, 01, 0f, 01, 02, 16, 03, 07, 0b, 00, 2e, 11, 01, 2b, 00, 2d, 07, 01, 0e, 00, 35, 0f, 02, 0b, 00, 2e, 2a, 01, 22, 00, 27, 26, 00, 2c, 00, 2e, 1f, 01, 0e, 00, 35, 23, 02, 01, 00, 02] Number of files: 1 - file 0 => global file 1 Number of expressions: 11 @@ -30,7 +30,7 @@ Number of expressions: 11 - expression 9 operands: lhs = Expression(10, Sub), rhs = Counter(6) - expression 10 operands: lhs = Counter(4), rhs = Counter(5) Number of file 0 mappings: 9 -- Code(Counter(0)) at (prev + 15, 1) to (start + 2, 25) +- Code(Counter(0)) at (prev + 15, 1) to (start + 2, 22) - Code(Expression(0, Add)) at (prev + 7, 11) to (start + 0, 46) = (c0 + Zero) - Code(Counter(4)) at (prev + 1, 43) to (start + 0, 45) diff --git a/tests/run-coverage/closure.coverage b/tests/run-coverage/closure.coverage index 930348dc431..67014f792c8 100644 --- a/tests/run-coverage/closure.coverage +++ b/tests/run-coverage/closure.coverage @@ -76,8 +76,8 @@ LL| 1| some_string = None; LL| 1| let LL| 1| a - LL| 1| = - LL| 1| || + LL| | = + LL| | || LL| 1| { LL| 1| let mut countdown = 0; LL| 1| if is_false { @@ -98,8 +98,8 @@ LL| 1| LL| 1| let LL| 1| quote_closure - LL| 1| = - LL| 1| |val| + LL| | = + LL| | |val| LL| 5| { LL| 5| let mut countdown = 0; LL| 5| if is_false { @@ -186,7 +186,7 @@ LL| | ; LL| | LL| 1| let short_used_not_covered_closure_line_break_block_embedded_branch = - LL| 1| | _unused_arg: u8 | + LL| | | _unused_arg: u8 | LL| 0| { LL| 0| println!( LL| 0| "not called: {}", @@ -196,7 +196,7 @@ LL| | ; LL| | LL| 1| let short_used_covered_closure_line_break_no_block_embedded_branch = - LL| 1| | _unused_arg: u8 | + LL| | | _unused_arg: u8 | LL| 1| println!( LL| 1| "not called: {}", LL| 1| if is_true { "check" } else { "me" } @@ -205,7 +205,7 @@ LL| | ; LL| | LL| 1| let short_used_covered_closure_line_break_block_embedded_branch = - LL| 1| | _unused_arg: u8 | + LL| | | _unused_arg: u8 | LL| 1| { LL| 1| println!( LL| 1| "not called: {}",