Skip to content

Commit

Permalink
Rollup merge of rust-lang#119460 - Zalathar:improper-region, r=wesley…
Browse files Browse the repository at this point in the history
…wiser

coverage: Never emit improperly-ordered coverage regions

If we emit a coverage region that is improperly ordered (end < start), `llvm-cov` will fail with `coveragemap_error::malformed`, which is inconvenient for users and also very hard to debug.

Ideally we would fix the root causes of these situations, but they tend to occur in very obscure edge-case scenarios (often involving nested macros), and we don't always have a good MCVE to work from. So it makes sense to also have a catch-all check that will prevent improperly-ordered regions from ever being emitted.

---

This is mainly aimed at resolving rust-lang#119453. We don't have a specific way to reproduce it, which is why I haven't been able to add a test case in this PR. But based on the information provided in that issue, this change seems likely to avoid the error in `llvm-cov`.

`````@rustbot````` label +A-code-coverage
  • Loading branch information
fmease authored Jan 24, 2024
2 parents 3529d45 + 21e5bea commit 5a38754
Showing 1 changed file with 34 additions and 1 deletion.
35 changes: 34 additions & 1 deletion compiler/rustc_mir_transform/src/coverage/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ fn make_code_region(
start_line = source_map.doctest_offset_line(&file.name, start_line);
end_line = source_map.doctest_offset_line(&file.name, end_line);

Some(CodeRegion {
check_code_region(CodeRegion {
file_name,
start_line: start_line as u32,
start_col: start_col as u32,
Expand All @@ -338,6 +338,39 @@ fn make_code_region(
})
}

/// If `llvm-cov` sees a code region that is improperly ordered (end < start),
/// it will immediately exit with a fatal error. To prevent that from happening,
/// discard regions that are improperly ordered, or might be interpreted in a
/// way that makes them improperly ordered.
fn check_code_region(code_region: CodeRegion) -> Option<CodeRegion> {
let CodeRegion { file_name: _, start_line, start_col, end_line, end_col } = code_region;

// Line/column coordinates are supposed to be 1-based. If we ever emit
// coordinates of 0, `llvm-cov` might misinterpret them.
let all_nonzero = [start_line, start_col, end_line, end_col].into_iter().all(|x| x != 0);
// Coverage mappings use the high bit of `end_col` to indicate that a
// region is actually a "gap" region, so make sure it's unset.
let end_col_has_high_bit_unset = (end_col & (1 << 31)) == 0;
// If a region is improperly ordered (end < start), `llvm-cov` will exit
// with a fatal error, which is inconvenient for users and hard to debug.
let is_ordered = (start_line, start_col) <= (end_line, end_col);

if all_nonzero && end_col_has_high_bit_unset && is_ordered {
Some(code_region)
} else {
debug!(
?code_region,
?all_nonzero,
?end_col_has_high_bit_unset,
?is_ordered,
"Skipping code region that would be misinterpreted or rejected by LLVM"
);
// If this happens in a debug build, ICE to make it easier to notice.
debug_assert!(false, "Improper code region: {code_region:?}");
None
}
}

fn is_eligible_for_coverage(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool {
// Only instrument functions, methods, and closures (not constants since they are evaluated
// at compile time by Miri).
Expand Down

0 comments on commit 5a38754

Please sign in to comment.