Skip to content

Commit

Permalink
Completes support for coverage in external crates
Browse files Browse the repository at this point in the history 
The prior PR corrected for errors encountered when trying to generate
the coverage map on source code inlined from external crates (including
macros and generics) by avoiding adding external DefIds to the coverage
map.

This made it possible to generate a coverage report including external
crates, but the external crate coverage was incomplete (did not include
coverage for the DefIds that were eliminated.

The root issue was that the coverage map was converting Span locations
to source file and locations, using the SourceMap for the current crate,
and this would not work for spans from external crates (compliled with a
different SourceMap).

The solution was to convert the Spans to filename and location during
MIR generation instead, so precompiled external crates would already
have the correct source code locations embedded in their MIR, when
imported into another crate.
  • Loading branch information
@richkadel
richkadel committed Aug 3, 2020
1 parent 5ef872f commit d55f927
Show file tree
Hide file tree
Showing 14 changed files with 352 additions and 325 deletions.
36 changes: 27 additions & 9 deletions library/core/src/intrinsics.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1950,15 +1950,20 @@ extern "rust-intrinsic" {
pub fn ptr_offset_from<T>(ptr: *const T, base: *const T) -> isize;

/// Internal placeholder for injecting code coverage counters when the "instrument-coverage"
/// option is enabled. The placeholder is replaced with `llvm.instrprof.increment` during code
/// generation.
/// option is enabled. The source code region information is extracted prior to code generation,
/// and added to the "coverage map", which is injected into the generated code as additional
/// data. This intrinsic then triggers the generation of LLVM intrinsic call
/// `instrprof.increment`, using the remaining args (`function_source_hash` and `index`).
#[cfg(not(bootstrap))]
#[lang = "count_code_region"]
pub fn count_code_region(
function_source_hash: u64,
index: u32,
start_byte_pos: u32,
end_byte_pos: u32,
file_name: &'static str,
start_line: u32,
start_col: u32,
end_line: u32,
end_col: u32,
);

/// Internal marker for code coverage expressions, injected into the MIR when the
Expand All @@ -1973,8 +1978,11 @@ extern "rust-intrinsic" {
index: u32,
left_index: u32,
right_index: u32,
start_byte_pos: u32,
end_byte_pos: u32,
file_name: &'static str,
start_line: u32,
start_col: u32,
end_line: u32,
end_col: u32,
);

/// This marker identifies a code region and two other counters or counter expressions
Expand All @@ -1986,14 +1994,24 @@ extern "rust-intrinsic" {
index: u32,
left_index: u32,
right_index: u32,
start_byte_pos: u32,
end_byte_pos: u32,
file_name: &'static str,
start_line: u32,
start_col: u32,
end_line: u32,
end_col: u32,
);

/// This marker identifies a code region to be added to the "coverage map" to indicate source
/// code that can never be reached.
/// (See `coverage_counter_add` for more information.)
pub fn coverage_unreachable(start_byte_pos: u32, end_byte_pos: u32);
#[cfg(not(bootstrap))]
pub fn coverage_unreachable(
file_name: &'static str,
start_line: u32,
start_col: u32,
end_line: u32,
end_col: u32,
);

/// See documentation of `<*const T>::guaranteed_eq` for details.
#[rustc_const_unstable(feature = "const_raw_ptr_comparison", issue = "53020")]
Expand Down
24 changes: 12 additions & 12 deletions src/librustc_codegen_llvm/coverageinfo/mapgen.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ impl CoverageMapGenerator {
fn write_coverage_mappings(
&mut self,
expressions: Vec<CounterExpression>,
counter_regions: impl Iterator<Item = (Counter, &'a Region)>,
counter_regions: impl Iterator<Item = (Counter, &'tcx Region<'tcx>)>,
coverage_mappings_buffer: &RustString,
) {
let mut counter_regions = counter_regions.collect::<Vec<_>>();
Expand All @@ -102,7 +102,7 @@ impl CoverageMapGenerator {

let mut virtual_file_mapping = Vec::new();
let mut mapping_regions = Vec::new();
let mut current_file_path = None;
let mut current_file_name = None;
let mut current_file_id = 0;

// Convert the list of (Counter, Region) pairs to an array of `CounterMappingRegion`, sorted
Expand All @@ -112,22 +112,22 @@ impl CoverageMapGenerator {
// `filenames` array.
counter_regions.sort_unstable_by_key(|(_counter, region)| *region);
for (counter, region) in counter_regions {
let (file_path, start_line, start_col, end_line, end_col) = region.file_start_and_end();
let same_file = current_file_path.as_ref().map_or(false, |p| p == file_path);
let Region { file_name, start_line, start_col, end_line, end_col } = *region;
let same_file = current_file_name.as_ref().map_or(false, |p| p == file_name);
if !same_file {
if current_file_path.is_some() {
if current_file_name.is_some() {
current_file_id += 1;
}
current_file_path = Some(file_path.clone());
let filename = CString::new(file_path.to_string_lossy().to_string())
.expect("null error converting filename to C string");
debug!(" file_id: {} = '{:?}'", current_file_id, filename);
let filenames_index = match self.filename_to_index.get(&filename) {
current_file_name = Some(file_name.to_string());
let c_filename =
CString::new(file_name).expect("null error converting filename to C string");
debug!(" file_id: {} = '{:?}'", current_file_id, c_filename);
let filenames_index = match self.filename_to_index.get(&c_filename) {
Some(index) => *index,
None => {
let index = self.filenames.len() as u32;
self.filenames.push(filename.clone());
self.filename_to_index.insert(filename.clone(), index);
self.filenames.push(c_filename.clone());
self.filename_to_index.insert(c_filename.clone(), index);
index
}
};
Expand Down
40 changes: 13 additions & 27 deletions src/librustc_codegen_llvm/coverageinfo/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::common::CodegenCx;
use libc::c_uint;
use llvm::coverageinfo::CounterMappingRegion;
use log::debug;
use rustc_codegen_ssa::coverageinfo::map::{CounterExpression, ExprKind, FunctionCoverage};
use rustc_codegen_ssa::coverageinfo::map::{CounterExpression, ExprKind, FunctionCoverage, Region};
use rustc_codegen_ssa::traits::{
BaseTypeMethods, CoverageInfoBuilderMethods, CoverageInfoMethods, StaticMethods,
};
Expand Down Expand Up @@ -49,19 +49,18 @@ impl CoverageInfoBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> {
instance: Instance<'tcx>,
function_source_hash: u64,
id: u32,
start_byte_pos: u32,
end_byte_pos: u32,
region: Region<'tcx>,
) {
debug!(
"adding counter to coverage_regions: instance={:?}, function_source_hash={}, id={}, \
byte range {}..{}",
instance, function_source_hash, id, start_byte_pos, end_byte_pos,
at {:?}",
instance, function_source_hash, id, region,
);
let mut coverage_regions = self.coverage_context().function_coverage_map.borrow_mut();
coverage_regions
.entry(instance)
.or_insert_with(|| FunctionCoverage::new(self.tcx, instance))
.add_counter(function_source_hash, id, start_byte_pos, end_byte_pos);
.add_counter(function_source_hash, id, region);
}

fn add_counter_expression_region(
Expand All @@ -71,43 +70,30 @@ impl CoverageInfoBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> {
lhs: u32,
op: ExprKind,
rhs: u32,
start_byte_pos: u32,
end_byte_pos: u32,
region: Region<'tcx>,
) {
debug!(
"adding counter expression to coverage_regions: instance={:?}, id={}, {} {:?} {}, \
byte range {}..{}",
instance, id_descending_from_max, lhs, op, rhs, start_byte_pos, end_byte_pos,
at {:?}",
instance, id_descending_from_max, lhs, op, rhs, region,
);
let mut coverage_regions = self.coverage_context().function_coverage_map.borrow_mut();
coverage_regions
.entry(instance)
.or_insert_with(|| FunctionCoverage::new(self.tcx, instance))
.add_counter_expression(
id_descending_from_max,
lhs,
op,
rhs,
start_byte_pos,
end_byte_pos,
);
.add_counter_expression(id_descending_from_max, lhs, op, rhs, region);
}

fn add_unreachable_region(
&mut self,
instance: Instance<'tcx>,
start_byte_pos: u32,
end_byte_pos: u32,
) {
fn add_unreachable_region(&mut self, instance: Instance<'tcx>, region: Region<'tcx>) {
debug!(
"adding unreachable code to coverage_regions: instance={:?}, byte range {}..{}",
instance, start_byte_pos, end_byte_pos,
"adding unreachable code to coverage_regions: instance={:?}, at {:?}",
instance, region,
);
let mut coverage_regions = self.coverage_context().function_coverage_map.borrow_mut();
coverage_regions
.entry(instance)
.or_insert_with(|| FunctionCoverage::new(self.tcx, instance))
.add_unreachable_region(start_byte_pos, end_byte_pos);
.add_unreachable_region(region);
}
}

Expand Down
121 changes: 61 additions & 60 deletions src/librustc_codegen_llvm/intrinsic.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use rustc_ast::ast;
use rustc_codegen_ssa::base::{compare_simd_types, to_immediate, wants_msvc_seh};
use rustc_codegen_ssa::common::span_invalid_monomorphization_error;
use rustc_codegen_ssa::common::{IntPredicate, TypeKind};
use rustc_codegen_ssa::coverageinfo::ExprKind;
use rustc_codegen_ssa::coverageinfo;
use rustc_codegen_ssa::glue;
use rustc_codegen_ssa::mir::operand::{OperandRef, OperandValue};
use rustc_codegen_ssa::mir::place::PlaceRef;
Expand Down Expand Up @@ -93,64 +93,64 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> {
let mut is_codegen_intrinsic = true;
// Set `is_codegen_intrinsic` to `false` to bypass `codegen_intrinsic_call()`.

if self.tcx.sess.opts.debugging_opts.instrument_coverage {
// If the intrinsic is from the local MIR, add the coverage information to the Codegen
// context, to be encoded into the local crate's coverage map.
if caller_instance.def_id().is_local() {
// FIXME(richkadel): Make sure to add coverage analysis tests on a crate with
// external crate dependencies, where:
// 1. Both binary and dependent crates are compiled with `-Zinstrument-coverage`
// 2. Only binary is compiled with `-Zinstrument-coverage`
// 3. Only dependent crates are compiled with `-Zinstrument-coverage`
match intrinsic {
sym::count_code_region => {
use coverage::count_code_region_args::*;
self.add_counter_region(
caller_instance,
op_to_u64(&args[FUNCTION_SOURCE_HASH]),
op_to_u32(&args[COUNTER_ID]),
op_to_u32(&args[START_BYTE_POS]),
op_to_u32(&args[END_BYTE_POS]),
);
}
sym::coverage_counter_add | sym::coverage_counter_subtract => {
use coverage::coverage_counter_expression_args::*;
self.add_counter_expression_region(
caller_instance,
op_to_u32(&args[EXPRESSION_ID]),
op_to_u32(&args[LEFT_ID]),
if intrinsic == sym::coverage_counter_add {
ExprKind::Add
} else {
ExprKind::Subtract
},
op_to_u32(&args[RIGHT_ID]),
op_to_u32(&args[START_BYTE_POS]),
op_to_u32(&args[END_BYTE_POS]),
);
}
sym::coverage_unreachable => {
use coverage::coverage_unreachable_args::*;
self.add_unreachable_region(
caller_instance,
op_to_u32(&args[START_BYTE_POS]),
op_to_u32(&args[END_BYTE_POS]),
);
}
_ => {}
}
// FIXME(richkadel): Make sure to add coverage analysis tests on a crate with
// external crate dependencies, where:
// 1. Both binary and dependent crates are compiled with `-Zinstrument-coverage`
// 2. Only binary is compiled with `-Zinstrument-coverage`
// 3. Only dependent crates are compiled with `-Zinstrument-coverage`
match intrinsic {
sym::count_code_region => {
use coverage::count_code_region_args::*;
self.add_counter_region(
caller_instance,
op_to_u64(&args[FUNCTION_SOURCE_HASH]),
op_to_u32(&args[COUNTER_ID]),
coverageinfo::Region::new(
op_to_str_slice(&args[FILE_NAME]),
op_to_u32(&args[START_LINE]),
op_to_u32(&args[START_COL]),
op_to_u32(&args[END_LINE]),
op_to_u32(&args[END_COL]),
),
);
}

// Only the `count_code_region` coverage intrinsic is translated into an actual LLVM
// intrinsic call (local or not); otherwise, set `is_codegen_intrinsic` to `false`.
match intrinsic {
sym::coverage_counter_add
| sym::coverage_counter_subtract
| sym::coverage_unreachable => {
is_codegen_intrinsic = false;
}
_ => {}
sym::coverage_counter_add | sym::coverage_counter_subtract => {
is_codegen_intrinsic = false;
use coverage::coverage_counter_expression_args::*;
self.add_counter_expression_region(
caller_instance,
op_to_u32(&args[EXPRESSION_ID]),
op_to_u32(&args[LEFT_ID]),
if intrinsic == sym::coverage_counter_add {
coverageinfo::ExprKind::Add
} else {
coverageinfo::ExprKind::Subtract
},
op_to_u32(&args[RIGHT_ID]),
coverageinfo::Region::new(
op_to_str_slice(&args[FILE_NAME]),
op_to_u32(&args[START_LINE]),
op_to_u32(&args[START_COL]),
op_to_u32(&args[END_LINE]),
op_to_u32(&args[END_COL]),
),
);
}
sym::coverage_unreachable => {
is_codegen_intrinsic = false;
use coverage::coverage_unreachable_args::*;
self.add_unreachable_region(
caller_instance,
coverageinfo::Region::new(
op_to_str_slice(&args[FILE_NAME]),
op_to_u32(&args[START_LINE]),
op_to_u32(&args[START_COL]),
op_to_u32(&args[END_LINE]),
op_to_u32(&args[END_COL]),
),
);
}
_ => {}
}
is_codegen_intrinsic
}
Expand Down Expand Up @@ -215,9 +215,6 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> {
self.call(llfn, &[], None)
}
sym::count_code_region => {
// FIXME(richkadel): The current implementation assumes the MIR for the given
// caller_instance represents a single function. Validate and/or correct if inlining
// and/or monomorphization invalidates these assumptions.
let coverageinfo = tcx.coverageinfo(caller_instance.def_id());
let mangled_fn = tcx.symbol_name(caller_instance);
let (mangled_fn_name, _len_val) = self.const_str(Symbol::intern(mangled_fn.name));
Expand Down Expand Up @@ -2283,6 +2280,10 @@ fn float_type_width(ty: Ty<'_>) -> Option<u64> {
}
}

fn op_to_str_slice<'tcx>(op: &Operand<'tcx>) -> &'tcx str {
Operand::value_from_const(op).try_to_str_slice().expect("Value is &str")
}

fn op_to_u32<'tcx>(op: &Operand<'tcx>) -> u32 {
Operand::scalar_from_const(op).to_u32().expect("Scalar is u32")
}
Expand Down
Loading

0 comments on commit d55f927

Please sign in to comment.