Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coverage: Rearrange the code for embedding per-function coverage metadata #134163

Merged
merged 5 commits into from
Dec 11, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
coverage: Only generate a CGU's covmap record if it has covfun records
  • Loading branch information
Zalathar committed Dec 11, 2024
commit 512f3fdebe72532a435238435f0e16eff61fbf38
18 changes: 14 additions & 4 deletions compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,6 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) {
let filenames_val = cx.const_bytes(&filenames_buffer);
let filenames_ref = llvm_cov::hash_bytes(&filenames_buffer);

// Generate the coverage map header, which contains the filenames used by
// this CGU's coverage mappings, and store it in a well-known global.
generate_covmap_record(cx, covmap_version, filenames_size, filenames_val);

let mut unused_function_names = Vec::new();

let covfun_records = function_coverage_map
Expand All @@ -93,6 +89,15 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) {
})
.collect::<Vec<_>>();

// If there are no covfun records for this CGU, don't generate a covmap record.
// Emitting a covmap record without any covfun records causes `llvm-cov` to
// fail when generating coverage reports, and if there are no covfun records
// then the covmap record isn't useful anyway.
// This should prevent a repeat of <https://github.com/rust-lang/rust/issues/133606>.
if covfun_records.is_empty() {
return;
}
Comment on lines +92 to +99
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it only occurred to me post-merge as I was looking at #134208. Did we ever have a regression test for this case (I imagine it can be hard to come with up with one)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be possible, but I would have to do some non-trivial detective work, because currently we don't have any examples smaller than “the coverage tests for my entire workspace, in CI”.


for covfun in &covfun_records {
SparrowLii marked this conversation as resolved.
Show resolved Hide resolved
unused_function_names.extend(covfun.mangled_function_name_if_unused());

Expand All @@ -117,6 +122,11 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) {
llvm::set_linkage(array, llvm::Linkage::InternalLinkage);
llvm::set_initializer(array, initializer);
}

// Generate the coverage map header, which contains the filenames used by
// this CGU's coverage mappings, and store it in a well-known global.
// (This is skipped if we returned early due to having no covfun records.)
generate_covmap_record(cx, covmap_version, filenames_size, filenames_val);
SparrowLii marked this conversation as resolved.
Show resolved Hide resolved
}

/// Maps "global" (per-CGU) file ID numbers to their underlying filenames.
Expand Down