Skip to content

Commit ebc0d0d

Browse files
wesleywiserwesleywiser
committed
Address review comments
1 parent ef57f24 commit ebc0d0d

File tree

4 files changed

+59
-36
lines changed

4 files changed

+59
-36
lines changed

‎compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -273,20 +273,34 @@ fn save_function_record(
273273
/// `codegened_and_inlined_items`).
274274
///
275275
/// These unused functions are then codegen'd in one of the CGUs which is marked as the
276-
/// "code coverage dead code cgu" during the partitioning process.
276+
/// "code coverage dead code cgu" during the partitioning process. This prevents us from generating
277+
/// code regions for the same function more than once which can lead to linker errors regarding
278+
/// duplicate symbols.
277279
fn add_unused_functions<'ll, 'tcx>(cx: &CodegenCx<'ll, 'tcx>) {
278280
assert!(cx.codegen_unit.is_code_coverage_dead_code_cgu());
279281

280282
let tcx = cx.tcx;
281283

282284
let ignore_unused_generics = tcx.sess.instrument_coverage_except_unused_generics();
283285

284-
let all_def_ids: DefIdSet = tcx
286+
let eligible_def_ids: DefIdSet = tcx
285287
.mir_keys(())
286288
.iter()
287289
.filter_map(|local_def_id| {
288290
let def_id = local_def_id.to_def_id();
289-
if ignore_unused_generics && tcx.generics_of(def_id).requires_monomorphization(tcx) {
291+
let kind = tcx.def_kind(def_id);
292+
// `mir_keys` will give us `DefId`s for all kinds of things, not
293+
// just "functions", like consts, statics, etc. Filter those out.
294+
// If `ignore_unused_generics` was specified, filter out any
295+
// generic functions from consideration as well.
296+
if !matches!(
297+
kind,
298+
DefKind::Fn | DefKind::AssocFn | DefKind::Closure | DefKind::Generator
299+
) {
300+
return None;
301+
} else if ignore_unused_generics
302+
&& tcx.generics_of(def_id).requires_monomorphization(tcx)
303+
{
290304
return None;
291305
}
292306
Some(local_def_id.to_def_id())
@@ -295,24 +309,17 @@ fn add_unused_functions<'ll, 'tcx>(cx: &CodegenCx<'ll, 'tcx>) {
295309

296310
let codegenned_def_ids = tcx.codegened_and_inlined_items(());
297311

298-
for &non_codegenned_def_id in all_def_ids.difference(codegenned_def_ids) {
299-
// `all_def_ids` contains things besides just "functions" such as constants,
300-
// statics, etc. We need to filter those out.
301-
let kind = tcx.def_kind(non_codegenned_def_id);
302-
if matches!(kind, DefKind::Fn | DefKind::AssocFn | DefKind::Closure | DefKind::Generator) {
303-
let codegen_fn_attrs = tcx.codegen_fn_attrs(non_codegenned_def_id);
304-
305-
// If a function is marked `#[no_coverage]`, then skip generating a
306-
// dead code stub for it.
307-
if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::NO_COVERAGE) {
308-
debug!("skipping unused fn marked #[no_coverage]: {:?}", non_codegenned_def_id);
309-
continue;
310-
}
312+
for &non_codegenned_def_id in eligible_def_ids.difference(codegenned_def_ids) {
313+
let codegen_fn_attrs = tcx.codegen_fn_attrs(non_codegenned_def_id);
311314

312-
debug!("generating unused fn: {:?}", non_codegenned_def_id);
313-
cx.define_unused_fn(non_codegenned_def_id);
314-
} else {
315-
debug!("skipping unused {:?}: {:?}", kind, non_codegenned_def_id);
315+
// If a function is marked `#[no_coverage]`, then skip generating a
316+
// dead code stub for it.
317+
if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::NO_COVERAGE) {
318+
debug!("skipping unused fn marked #[no_coverage]: {:?}", non_codegenned_def_id);
319+
continue;
316320
}
321+
322+
debug!("generating unused fn: {:?}", non_codegenned_def_id);
323+
cx.define_unused_fn(non_codegenned_def_id);
317324
}
318325
}

‎compiler/rustc_monomorphize/src/partitioning/mod.rs

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -205,17 +205,33 @@ pub fn partition<'tcx>(
205205
tcx.sess.instrument_coverage() && !tcx.sess.instrument_coverage_except_unused_functions();
206206

207207
if instrument_dead_code {
208+
assert!(
209+
post_inlining.codegen_units.len() > 0,
210+
"There must be at least one CGU that code coverage data can be generated in."
211+
);
212+
208213
// Find the smallest CGU that has exported symbols and put the dead
209214
// function stubs in that CGU. We look for exported symbols to increase
210-
// the likelyhood the linker won't throw away the dead functions.
211-
let mut cgus_with_exported_symbols: Vec<_> = post_inlining
212-
.codegen_units
213-
.iter_mut()
215+
// the likelihood the linker won't throw away the dead functions.
216+
// FIXME(#92165): In order to truly resolve this, we need to make sure
217+
// the object file (CGU) containing the dead function stubs is included
218+
// in the final binary. This will probably require forcing these
219+
// function symbols to be included via `-u` or `/include` linker args.
220+
let mut cgus: Vec<_> = post_inlining.codegen_units.iter_mut().collect();
221+
cgus.sort_by_key(|cgu| cgu.size_estimate());
222+
223+
let dead_code_cgu = if let Some(cgu) = cgus
224+
.into_iter()
225+
.rev()
214226
.filter(|cgu| cgu.items().iter().any(|(_, (linkage, _))| *linkage == Linkage::External))
215-
.collect();
216-
cgus_with_exported_symbols.sort_by_key(|cgu| cgu.size_estimate());
217-
218-
let dead_code_cgu = cgus_with_exported_symbols.last_mut().unwrap();
227+
.next()
228+
{
229+
cgu
230+
} else {
231+
// If there are no CGUs that have externally linked items,
232+
// then we just pick the first CGU as a fallback.
233+
&mut post_inlining.codegen_units[0]
234+
};
219235
dead_code_cgu.make_code_coverage_dead_code_cgu();
220236
}
221237

‎src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,12 @@
1919
18| 2| println!("used_only_from_bin_crate_generic_function with {:?}", arg);
2020
19| 2|}
2121
------------------
22-
| used_crate::used_only_from_bin_crate_generic_function::<&str>:
22+
| used_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec<i32>>:
2323
| 17| 1|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
2424
| 18| 1| println!("used_only_from_bin_crate_generic_function with {:?}", arg);
2525
| 19| 1|}
2626
------------------
27-
| used_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec<i32>>:
27+
| used_crate::used_only_from_bin_crate_generic_function::<&str>:
2828
| 17| 1|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
2929
| 18| 1| println!("used_only_from_bin_crate_generic_function with {:?}", arg);
3030
| 19| 1|}
@@ -36,12 +36,12 @@
3636
22| 2| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
3737
23| 2|}
3838
------------------
39-
| used_crate::used_only_from_this_lib_crate_generic_function::<&str>:
39+
| used_crate::used_only_from_this_lib_crate_generic_function::<alloc::vec::Vec<i32>>:
4040
| 21| 1|pub fn used_only_from_this_lib_crate_generic_function<T: Debug>(arg: T) {
4141
| 22| 1| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
4242
| 23| 1|}
4343
------------------
44-
| used_crate::used_only_from_this_lib_crate_generic_function::<alloc::vec::Vec<i32>>:
44+
| used_crate::used_only_from_this_lib_crate_generic_function::<&str>:
4545
| 21| 1|pub fn used_only_from_this_lib_crate_generic_function<T: Debug>(arg: T) {
4646
| 22| 1| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
4747
| 23| 1|}

‎src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_inline_crate.txt

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,12 +42,12 @@
4242
40| 2| println!("used_only_from_bin_crate_generic_function with {:?}", arg);
4343
41| 2|}
4444
------------------
45-
| used_inline_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec<i32>>:
45+
| used_inline_crate::used_only_from_bin_crate_generic_function::<&str>:
4646
| 39| 1|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
4747
| 40| 1| println!("used_only_from_bin_crate_generic_function with {:?}", arg);
4848
| 41| 1|}
4949
------------------
50-
| used_inline_crate::used_only_from_bin_crate_generic_function::<&str>:
50+
| used_inline_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec<i32>>:
5151
| 39| 1|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
5252
| 40| 1| println!("used_only_from_bin_crate_generic_function with {:?}", arg);
5353
| 41| 1|}
@@ -61,12 +61,12 @@
6161
46| 4| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
6262
47| 4|}
6363
------------------
64-
| used_inline_crate::used_only_from_this_lib_crate_generic_function::<alloc::vec::Vec<i32>>:
64+
| used_inline_crate::used_only_from_this_lib_crate_generic_function::<&str>:
6565
| 45| 2|pub fn used_only_from_this_lib_crate_generic_function<T: Debug>(arg: T) {
6666
| 46| 2| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
6767
| 47| 2|}
6868
------------------
69-
| used_inline_crate::used_only_from_this_lib_crate_generic_function::<&str>:
69+
| used_inline_crate::used_only_from_this_lib_crate_generic_function::<alloc::vec::Vec<i32>>:
7070
| 45| 2|pub fn used_only_from_this_lib_crate_generic_function<T: Debug>(arg: T) {
7171
| 46| 2| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg);
7272
| 47| 2|}

0 commit comments

Comments
 (0)