Skip to content

Commit 34b26d6

Browse files
richkadelrichkadel
committed
Rust function-level coverage now works on external crates
Fixed a known issue in the coverage map where some regions had nonsensical source code locations. External crate functions are already included in their own coverage maps, per library, and don't need to also be added to the importing crate's coverage map. (In fact, their source start and end byte positions are not relevant to the importing crate's SourceMap.) The fix was to simply skip trying to add imported coverage info to the coverage map if the instrumented function is not "local". The injected counters are still relevant, however, and the LLVM `instrprof.increment` intrinsic call parameters will map those counters to the external crates' coverage maps, when generating runtime coverage data.
1 parent 6b09c37 commit 34b26d6

File tree

2 files changed

+62
-53
lines changed

2 files changed

+62
-53
lines changed

‎src/librustc_codegen_llvm/intrinsic.rs

+57-52
Original file line numberDiff line numberDiff line change
@@ -90,64 +90,69 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> {
9090
args: &Vec<Operand<'tcx>>,
9191
caller_instance: ty::Instance<'tcx>,
9292
) -> bool {
93+
let mut is_codegen_intrinsic = true;
94+
// Set `is_codegen_intrinsic` to `false` to bypass `codegen_intrinsic_call()`.
95+
9396
if self.tcx.sess.opts.debugging_opts.instrument_coverage {
94-
// Add the coverage information from the MIR to the Codegen context. Some coverage
95-
// intrinsics are used only to pass along the coverage information (returns `false`
96-
// for `is_codegen_intrinsic()`), but `count_code_region` is also converted into an
97-
// LLVM intrinsic to increment a coverage counter.
98-
match intrinsic {
99-
sym::count_code_region => {
100-
use coverage::count_code_region_args::*;
101-
self.add_counter_region(
102-
caller_instance,
103-
op_to_u64(&args[FUNCTION_SOURCE_HASH]),
104-
op_to_u32(&args[COUNTER_ID]),
105-
op_to_u32(&args[START_BYTE_POS]),
106-
op_to_u32(&args[END_BYTE_POS]),
107-
);
108-
return true; // Also inject the counter increment in the backend
109-
}
110-
sym::coverage_counter_add | sym::coverage_counter_subtract => {
111-
use coverage::coverage_counter_expression_args::*;
112-
self.add_counter_expression_region(
113-
caller_instance,
114-
op_to_u32(&args[EXPRESSION_ID]),
115-
op_to_u32(&args[LEFT_ID]),
116-
if intrinsic == sym::coverage_counter_add {
117-
ExprKind::Add
118-
} else {
119-
ExprKind::Subtract
120-
},
121-
op_to_u32(&args[RIGHT_ID]),
122-
op_to_u32(&args[START_BYTE_POS]),
123-
op_to_u32(&args[END_BYTE_POS]),
124-
);
125-
return false; // Does not inject backend code
97+
// If the intrinsic is from the local MIR, add the coverage information to the Codegen
98+
// context, to be encoded into the local crate's coverage map.
99+
if caller_instance.def_id().is_local() {
100+
// FIXME(richkadel): Make sure to add coverage analysis tests on a crate with
101+
// external crate dependencies, where:
102+
// 1. Both binary and dependent crates are compiled with `-Zinstrument-coverage`
103+
// 2. Only binary is compiled with `-Zinstrument-coverage`
104+
// 3. Only dependent crates are compiled with `-Zinstrument-coverage`
105+
match intrinsic {
106+
sym::count_code_region => {
107+
use coverage::count_code_region_args::*;
108+
self.add_counter_region(
109+
caller_instance,
110+
op_to_u64(&args[FUNCTION_SOURCE_HASH]),
111+
op_to_u32(&args[COUNTER_ID]),
112+
op_to_u32(&args[START_BYTE_POS]),
113+
op_to_u32(&args[END_BYTE_POS]),
114+
);
115+
}
116+
sym::coverage_counter_add | sym::coverage_counter_subtract => {
117+
use coverage::coverage_counter_expression_args::*;
118+
self.add_counter_expression_region(
119+
caller_instance,
120+
op_to_u32(&args[EXPRESSION_ID]),
121+
op_to_u32(&args[LEFT_ID]),
122+
if intrinsic == sym::coverage_counter_add {
123+
ExprKind::Add
124+
} else {
125+
ExprKind::Subtract
126+
},
127+
op_to_u32(&args[RIGHT_ID]),
128+
op_to_u32(&args[START_BYTE_POS]),
129+
op_to_u32(&args[END_BYTE_POS]),
130+
);
131+
}
132+
sym::coverage_unreachable => {
133+
use coverage::coverage_unreachable_args::*;
134+
self.add_unreachable_region(
135+
caller_instance,
136+
op_to_u32(&args[START_BYTE_POS]),
137+
op_to_u32(&args[END_BYTE_POS]),
138+
);
139+
}
140+
_ => {}
126141
}
127-
sym::coverage_unreachable => {
128-
use coverage::coverage_unreachable_args::*;
129-
self.add_unreachable_region(
130-
caller_instance,
131-
op_to_u32(&args[START_BYTE_POS]),
132-
op_to_u32(&args[END_BYTE_POS]),
133-
);
134-
return false; // Does not inject backend code
142+
}
143+
144+
// Only the `count_code_region` coverage intrinsic is translated into an actual LLVM
145+
// intrinsic call (local or not); otherwise, set `is_codegen_intrinsic` to `false`.
146+
match intrinsic {
147+
sym::coverage_counter_add
148+
| sym::coverage_counter_subtract
149+
| sym::coverage_unreachable => {
150+
is_codegen_intrinsic = false;
135151
}
136152
_ => {}
137153
}
138-
} else {
139-
// NOT self.tcx.sess.opts.debugging_opts.instrument_coverage
140-
if intrinsic == sym::count_code_region {
141-
// An external crate may have been pre-compiled with coverage instrumentation, and
142-
// some references from the current crate to the external crate might carry along
143-
// the call terminators to coverage intrinsics, like `count_code_region` (for
144-
// example, when instantiating a generic function). If the current crate has
145-
// `instrument_coverage` disabled, the `count_code_region` call terminators should
146-
// be ignored.
147-
return false; // Do not inject coverage counters inlined from external crates
148-
}
149154
}
150-
true // Unhandled intrinsics should be passed to `codegen_intrinsic_call()`
155+
is_codegen_intrinsic
151156
}
152157

153158
fn codegen_intrinsic_call(

‎src/librustc_codegen_ssa/coverageinfo/map.rs

+5-1
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,11 @@ impl Region {
8686
pub fn new(source_map: &SourceMap, start_byte_pos: u32, end_byte_pos: u32) -> Self {
8787
let start = source_map.lookup_char_pos(BytePos::from_u32(start_byte_pos));
8888
let end = source_map.lookup_char_pos(BytePos::from_u32(end_byte_pos));
89-
assert_eq!(start.file.name, end.file.name);
89+
assert_eq!(
90+
start.file.name, end.file.name,
91+
"Region start ({} -> {:?}) and end ({} -> {:?}) don't come from the same source file!",
92+
start_byte_pos, start, end_byte_pos, end
93+
);
9094
Self { start, end }
9195
}
9296

0 commit comments

Comments
 (0)