From 888d0b4c9691ffcba7cb326a1cd1fc73feb38f57 Mon Sep 17 00:00:00 2001
From: Rich Kadel <richkadel@google.com>
Date: Sun, 25 Apr 2021 13:34:03 -0700
Subject: [PATCH] Derived Eq no longer shows uncovered
The Eq trait has a special hidden function. MIR `InstrumentCoverage`
would add this function to the coverage map, but it is never called, so
the `Eq` trait would always appear uncovered.
Fixes: #83601
The fix required creating a new function attribute `no_coverage` to mark
functions that should be ignored by `InstrumentCoverage` and the
coverage `mapgen` (during codegen).
While testing, I also noticed two other issues:
* spanview debug file output ICEd on a function with no body. The
workaround for this is included in this PR.
* `assert_*!()` macro coverage can appear covered if followed by another
`assert_*!()` macro. Normally they appear uncovered. I submitted a new
Issue #84561, and added a coverage test to demonstrate this issue.
---
.../src/deriving/cmp/eq.rs | 3 +-
.../src/coverageinfo/mapgen.rs | 5 +++
compiler/rustc_feature/src/builtin_attrs.rs | 1 +
.../src/middle/codegen_fn_attrs.rs | 4 +++
.../rustc_mir/src/transform/coverage/mod.rs | 6 ++++
compiler/rustc_span/src/symbol.rs | 1 +
compiler/rustc_typeck/src/collect.rs | 2 ++
library/core/src/cmp.rs | 1 +
.../expected_show_coverage.issue-83601.txt | 22 ++++++++++++
.../expected_show_coverage.issue-84561.txt | 34 +++++++++++++++++++
.../expected_show_coverage.partial_eq.txt | 2 +-
.../run-make-fulldeps/coverage/issue-83601.rs | 14 ++++++++
.../run-make-fulldeps/coverage/issue-84561.rs | 26 ++++++++++++++
13 files changed, 119 insertions(+), 2 deletions(-)
create mode 100644 src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-83601.txt
create mode 100644 src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-84561.txt
create mode 100644 src/test/run-make-fulldeps/coverage/issue-83601.rs
create mode 100644 src/test/run-make-fulldeps/coverage/issue-84561.rs
diff --git a/compiler/rustc_builtin_macros/src/deriving/cmp/eq.rs b/compiler/rustc_builtin_macros/src/deriving/cmp/eq.rs
index 79f35ad5819..53e9681b437 100644
--- a/compiler/rustc_builtin_macros/src/deriving/cmp/eq.rs
+++ b/compiler/rustc_builtin_macros/src/deriving/cmp/eq.rs
@@ -16,9 +16,10 @@ pub fn expand_deriving_eq(
push: &mut dyn FnMut(Annotatable),
) {
let inline = cx.meta_word(span, sym::inline);
+ let no_coverage = cx.meta_word(span, sym::no_coverage);
let hidden = rustc_ast::attr::mk_nested_word_item(Ident::new(sym::hidden, span));
let doc = rustc_ast::attr::mk_list_item(Ident::new(sym::doc, span), vec![hidden]);
- let attrs = vec![cx.attribute(inline), cx.attribute(doc)];
+ let attrs = vec![cx.attribute(inline), cx.attribute(no_coverage), cx.attribute(doc)];
let trait_def = TraitDef {
span,
attributes: Vec::new(),
diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs
index 2ac814bf228..1faaa7e86f6 100644
--- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs
+++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs
@@ -8,6 +8,7 @@ use rustc_codegen_ssa::traits::{ConstMethods, CoverageInfoMethods};
use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexSet};
use rustc_hir::def_id::{DefId, DefIdSet, LOCAL_CRATE};
use rustc_llvm::RustString;
+use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags;
use rustc_middle::mir::coverage::CodeRegion;
use rustc_span::Symbol;
@@ -280,6 +281,10 @@ fn add_unused_functions<'ll, 'tcx>(cx: &CodegenCx<'ll, 'tcx>) {
let mut unused_def_ids_by_file: FxHashMap<Symbol, Vec<DefId>> = FxHashMap::default();
for &non_codegenned_def_id in all_def_ids.difference(codegenned_def_ids) {
+ let codegen_fn_attrs = tcx.codegen_fn_attrs(non_codegenned_def_id);
+ if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::NO_COVERAGE) {
+ continue;
+ }
// Make sure the non-codegenned (unused) function has a file_name
if let Some(non_codegenned_file_name) = tcx.covered_file_name(non_codegenned_def_id) {
let def_ids =
diff --git a/compiler/rustc_feature/src/builtin_attrs.rs b/compiler/rustc_feature/src/builtin_attrs.rs
index b8a0b8debcd..b35072933d9 100644
--- a/compiler/rustc_feature/src/builtin_attrs.rs
+++ b/compiler/rustc_feature/src/builtin_attrs.rs
@@ -264,6 +264,7 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
// Code generation:
ungated!(inline, AssumedUsed, template!(Word, List: "always|never")),
+ ungated!(no_coverage, AssumedUsed, template!(Word)),
ungated!(cold, AssumedUsed, template!(Word)),
ungated!(no_builtins, AssumedUsed, template!(Word)),
ungated!(target_feature, AssumedUsed, template!(List: r#"enable = "name""#)),
diff --git a/compiler/rustc_middle/src/middle/codegen_fn_attrs.rs b/compiler/rustc_middle/src/middle/codegen_fn_attrs.rs
index 7024d9a3d21..93e7aeaffce 100644
--- a/compiler/rustc_middle/src/middle/codegen_fn_attrs.rs
+++ b/compiler/rustc_middle/src/middle/codegen_fn_attrs.rs
@@ -89,6 +89,10 @@ bitflags! {
/// #[cmse_nonsecure_entry]: with a TrustZone-M extension, declare a
/// function as an entry function from Non-Secure code.
const CMSE_NONSECURE_ENTRY = 1 << 14;
+ /// `#[no_coverage]`: indicates that the function should be ignored by
+ /// the MIR `InstrumentCoverage` pass and not added to the coverage map
+ /// during codegen.
+ const NO_COVERAGE = 1 << 15;
}
}
diff --git a/compiler/rustc_mir/src/transform/coverage/mod.rs b/compiler/rustc_mir/src/transform/coverage/mod.rs
index 3ef03ec21ad..eaeb44289cf 100644
--- a/compiler/rustc_mir/src/transform/coverage/mod.rs
+++ b/compiler/rustc_mir/src/transform/coverage/mod.rs
@@ -23,6 +23,7 @@ use rustc_index::vec::IndexVec;
use rustc_middle::hir;
use rustc_middle::hir::map::blocks::FnLikeNode;
use rustc_middle::ich::StableHashingContext;
+use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags;
use rustc_middle::mir::coverage::*;
use rustc_middle::mir::{
self, BasicBlock, BasicBlockData, Coverage, SourceInfo, Statement, StatementKind, Terminator,
@@ -87,6 +88,11 @@ impl<'tcx> MirPass<'tcx> for InstrumentCoverage {
_ => {}
}
+ let codegen_fn_attrs = tcx.codegen_fn_attrs(mir_source.def_id());
+ if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::NO_COVERAGE) {
+ return;
+ }
+
trace!("InstrumentCoverage starting for {:?}", mir_source.def_id());
Instrumentor::new(&self.name(), tcx, mir_body).inject_counters();
trace!("InstrumentCoverage starting for {:?}", mir_source.def_id());
diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs
index 4be187c5208..cb6c72b1317 100644
--- a/compiler/rustc_span/src/symbol.rs
+++ b/compiler/rustc_span/src/symbol.rs
@@ -781,6 +781,7 @@ symbols! {
no,
no_builtins,
no_core,
+ no_coverage,
no_crate_inject,
no_debug,
no_default_passes,
diff --git a/compiler/rustc_typeck/src/collect.rs b/compiler/rustc_typeck/src/collect.rs
index 46ee8245432..0528f8812f9 100644
--- a/compiler/rustc_typeck/src/collect.rs
+++ b/compiler/rustc_typeck/src/collect.rs
@@ -2724,6 +2724,8 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, id: DefId) -> CodegenFnAttrs {
codegen_fn_attrs.flags |= CodegenFnAttrFlags::NAKED;
} else if tcx.sess.check_name(attr, sym::no_mangle) {
codegen_fn_attrs.flags |= CodegenFnAttrFlags::NO_MANGLE;
+ } else if tcx.sess.check_name(attr, sym::no_coverage) {
+ codegen_fn_attrs.flags |= CodegenFnAttrFlags::NO_COVERAGE;
} else if tcx.sess.check_name(attr, sym::rustc_std_internal_symbol) {
codegen_fn_attrs.flags |= CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL;
} else if tcx.sess.check_name(attr, sym::used) {
diff --git a/library/core/src/cmp.rs b/library/core/src/cmp.rs
index cdb6006b1b3..1f7ae7e3773 100644
--- a/library/core/src/cmp.rs
+++ b/library/core/src/cmp.rs
@@ -274,6 +274,7 @@ pub trait Eq: PartialEq<Self> {
//
// This should never be implemented by hand.
#[doc(hidden)]
+ #[cfg_attr(not(bootstrap), no_coverage)]
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
fn assert_receiver_is_total_eq(&self) {}
diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-83601.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-83601.txt
new file mode 100644
index 00000000000..46f3add9427
--- /dev/null
+++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-83601.txt
@@ -0,0 +1,22 @@
+ 1| |// Shows that rust-lang/rust/83601 is resolved
+ 2| |
+ 3| 3|#[derive(Debug, PartialEq, Eq)]
+ ^2
+ ------------------
+ | <issue_83601::Foo as core::cmp::PartialEq>::eq:
+ | 3| 2|#[derive(Debug, PartialEq, Eq)]
+ ------------------
+ | Unexecuted instantiation: <issue_83601::Foo as core::cmp::PartialEq>::ne
+ ------------------
+ 4| |struct Foo(u32);
+ 5| |
+ 6| 1|fn main() {
+ 7| 1| let bar = Foo(1);
+ 8| 0| assert_eq!(bar, Foo(1));
+ 9| 1| let baz = Foo(0);
+ 10| 0| assert_ne!(baz, Foo(1));
+ 11| 1| println!("{:?}", Foo(1));
+ 12| 1| println!("{:?}", bar);
+ 13| 1| println!("{:?}", baz);
+ 14| 1|}
+
diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-84561.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-84561.txt
new file mode 100644
index 00000000000..faafa828817
--- /dev/null
+++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-84561.txt
@@ -0,0 +1,34 @@
+ 1| |// FIXME(#84561): function-like macros produce unintuitive coverage results.
+ 2| |// This test demonstrates some of the problems.
+ 3| |
+ 4| 9|#[derive(Debug, PartialEq, Eq)]
+ ^5
+ ------------------
+ | <issue_84561::Foo as core::cmp::PartialEq>::eq:
+ | 4| 9|#[derive(Debug, PartialEq, Eq)]
+ ------------------
+ | Unexecuted instantiation: <issue_84561::Foo as core::cmp::PartialEq>::ne
+ ------------------
+ 5| |struct Foo(u32);
+ 6| |
+ 7| 1|fn main() {
+ 8| 1| let bar = Foo(1);
+ 9| 0| assert_eq!(bar, Foo(1));
+ 10| 1| let baz = Foo(0);
+ 11| 0| assert_ne!(baz, Foo(1));
+ 12| 1| println!("{:?}", Foo(1));
+ 13| 1| println!("{:?}", bar);
+ 14| 1| println!("{:?}", baz);
+ 15| |
+ 16| 1| assert_eq!(Foo(1), Foo(1));
+ 17| 1| assert_ne!(Foo(0), Foo(1));
+ 18| 0| assert_eq!(Foo(2), Foo(2));
+ 19| 1| let bar = Foo(1);
+ 20| 1| assert_ne!(Foo(0), Foo(3));
+ 21| 1| assert_ne!(Foo(0), Foo(4));
+ 22| 1| assert_eq!(Foo(3), Foo(3));
+ 23| 0| assert_ne!(Foo(0), Foo(5));
+ 24| 1| println!("{:?}", bar);
+ 25| 1| println!("{:?}", Foo(1));
+ 26| 1|}
+
diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.partial_eq.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.partial_eq.txt
index 4e4dde46b34..fc266653349 100644
--- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.partial_eq.txt
+++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.partial_eq.txt
@@ -2,7 +2,7 @@
2| |// structure of this test.
3| |
4| 2|#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
- ^0 ^0 ^0 ^0 ^1 ^1 ^0^0
+ ^0 ^0 ^0 ^1 ^1 ^0^0
------------------
| Unexecuted instantiation: <partial_eq::Version as core::cmp::PartialEq>::ne
------------------
diff --git a/src/test/run-make-fulldeps/coverage/issue-83601.rs b/src/test/run-make-fulldeps/coverage/issue-83601.rs
new file mode 100644
index 00000000000..0b72a81947c
--- /dev/null
+++ b/src/test/run-make-fulldeps/coverage/issue-83601.rs
@@ -0,0 +1,14 @@
+// Shows that rust-lang/rust/83601 is resolved
+
+#[derive(Debug, PartialEq, Eq)]
+struct Foo(u32);
+
+fn main() {
+ let bar = Foo(1);
+ assert_eq!(bar, Foo(1));
+ let baz = Foo(0);
+ assert_ne!(baz, Foo(1));
+ println!("{:?}", Foo(1));
+ println!("{:?}", bar);
+ println!("{:?}", baz);
+}
diff --git a/src/test/run-make-fulldeps/coverage/issue-84561.rs b/src/test/run-make-fulldeps/coverage/issue-84561.rs
new file mode 100644
index 00000000000..dc183685e3d
--- /dev/null
+++ b/src/test/run-make-fulldeps/coverage/issue-84561.rs
@@ -0,0 +1,26 @@
+// FIXME(#84561): function-like macros produce unintuitive coverage results.
+// This test demonstrates some of the problems.
+
+#[derive(Debug, PartialEq, Eq)]
+struct Foo(u32);
+
+fn main() {
+ let bar = Foo(1);
+ assert_eq!(bar, Foo(1));
+ let baz = Foo(0);
+ assert_ne!(baz, Foo(1));
+ println!("{:?}", Foo(1));
+ println!("{:?}", bar);
+ println!("{:?}", baz);
+
+ assert_eq!(Foo(1), Foo(1));
+ assert_ne!(Foo(0), Foo(1));
+ assert_eq!(Foo(2), Foo(2));
+ let bar = Foo(1);
+ assert_ne!(Foo(0), Foo(3));
+ assert_ne!(Foo(0), Foo(4));
+ assert_eq!(Foo(3), Foo(3));
+ assert_ne!(Foo(0), Foo(5));
+ println!("{:?}", bar);
+ println!("{:?}", Foo(1));
+}