From d34be935ecdcd24d9623ceb32e3cb3e40996524c Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 19 Apr 2024 23:09:57 +0200 Subject: [PATCH 01/13] Prevent creating the same `Instance::mono` multiple times --- compiler/rustc_codegen_llvm/src/consts.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/consts.rs b/compiler/rustc_codegen_llvm/src/consts.rs index 4afa230e598b7..3e0597b1a76d0 100644 --- a/compiler/rustc_codegen_llvm/src/consts.rs +++ b/compiler/rustc_codegen_llvm/src/consts.rs @@ -246,7 +246,8 @@ impl<'ll> CodegenCx<'ll, '_> { #[instrument(level = "debug", skip(self, llty))] pub(crate) fn get_static_inner(&self, def_id: DefId, llty: &'ll Type) -> &'ll Value { - if let Some(&g) = self.instances.borrow().get(&Instance::mono(self.tcx, def_id)) { + let instance = Instance::mono(self.tcx, def_id); + if let Some(&g) = self.instances.borrow().get(&instance) { trace!("used cached value"); return g; } @@ -259,7 +260,7 @@ impl<'ll> CodegenCx<'ll, '_> { statics defined in the same CGU, but did not for `{def_id:?}`" ); - let sym = self.tcx.symbol_name(Instance::mono(self.tcx, def_id)).name; + let sym = self.tcx.symbol_name(instance).name; let fn_attrs = self.tcx.codegen_fn_attrs(def_id); debug!(?sym, ?fn_attrs); @@ -349,7 +350,7 @@ impl<'ll> CodegenCx<'ll, '_> { } } - self.instances.borrow_mut().insert(Instance::mono(self.tcx, def_id), g); + self.instances.borrow_mut().insert(instance, g); g } From 86756c18046708e0065cddd177aed8d2dcb5d6f0 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Fri, 19 Apr 2024 21:09:51 -0400 Subject: [PATCH 02/13] Stop taking ParamTy/ParamConst/EarlyParamRegion/AliasTy by ref --- .../src/diagnostics/conflict_errors.rs | 2 +- compiler/rustc_borrowck/src/diagnostics/mod.rs | 2 +- compiler/rustc_hir_typeck/src/expr.rs | 2 +- compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs | 2 +- compiler/rustc_hir_typeck/src/method/suggest.rs | 14 +++++++------- .../rustc_infer/src/infer/error_reporting/mod.rs | 2 +- .../src/infer/error_reporting/note_and_explain.rs | 12 ++++++------ compiler/rustc_middle/src/ty/generics.rs | 8 ++++---- compiler/rustc_middle/src/ty/print/pretty.rs | 4 ++-- compiler/rustc_middle/src/ty/sty.rs | 2 +- compiler/rustc_middle/src/ty/util.rs | 8 ++++---- .../src/traits/error_reporting/suggestions.rs | 6 +++--- compiler/rustc_trait_selection/src/traits/mod.rs | 6 +++--- compiler/rustc_ty_utils/src/opaque_types.rs | 6 +++--- 14 files changed, 38 insertions(+), 38 deletions(-) diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs index ec0d4af599ea5..61cd085eef5b3 100644 --- a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs @@ -1320,7 +1320,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { .into_iter() .map(|err| match err.obligation.predicate.kind().skip_binder() { PredicateKind::Clause(ty::ClauseKind::Trait(predicate)) => { - match predicate.self_ty().kind() { + match *predicate.self_ty().kind() { ty::Param(param_ty) => Ok(( generics.type_param(param_ty, tcx), predicate.trait_ref.print_only_trait_path().to_string(), diff --git a/compiler/rustc_borrowck/src/diagnostics/mod.rs b/compiler/rustc_borrowck/src/diagnostics/mod.rs index dbea317e7bb26..a505924fabdad 100644 --- a/compiler/rustc_borrowck/src/diagnostics/mod.rs +++ b/compiler/rustc_borrowck/src/diagnostics/mod.rs @@ -1070,7 +1070,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { // LL | blk(); // | ----- this value implements `FnOnce`, which causes it to be moved when called // ``` - if let ty::Param(param_ty) = self_ty.kind() + if let ty::Param(param_ty) = *self_ty.kind() && let generics = self.infcx.tcx.generics_of(self.mir_def_id()) && let param = generics.type_param(param_ty, self.infcx.tcx) && let Some(hir_generics) = self diff --git a/compiler/rustc_hir_typeck/src/expr.rs b/compiler/rustc_hir_typeck/src/expr.rs index 8923137fdd82d..48b9142b01424 100644 --- a/compiler/rustc_hir_typeck/src/expr.rs +++ b/compiler/rustc_hir_typeck/src/expr.rs @@ -2697,7 +2697,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { fn point_at_param_definition(&self, err: &mut Diag<'_>, param: ty::ParamTy) { let generics = self.tcx.generics_of(self.body_id); - let generic_param = generics.type_param(¶m, self.tcx); + let generic_param = generics.type_param(param, self.tcx); if let ty::GenericParamDefKind::Type { synthetic: true, .. } = generic_param.kind { return; } diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs index d9e5289f6323b..b729fbe844a3a 100644 --- a/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs +++ b/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs @@ -2144,7 +2144,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let callee_ty = callee_ty.peel_refs(); match *callee_ty.kind() { ty::Param(param) => { - let param = self.tcx.generics_of(self.body_id).type_param(¶m, self.tcx); + let param = self.tcx.generics_of(self.body_id).type_param(param, self.tcx); if param.kind.is_synthetic() { // if it's `impl Fn() -> ..` then just fall down to the def-id based logic def_id = param.def_id; diff --git a/compiler/rustc_hir_typeck/src/method/suggest.rs b/compiler/rustc_hir_typeck/src/method/suggest.rs index 46227e406a328..e74c4d06b5b27 100644 --- a/compiler/rustc_hir_typeck/src/method/suggest.rs +++ b/compiler/rustc_hir_typeck/src/method/suggest.rs @@ -125,11 +125,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let Some(arg_ty) = args[0].as_type() else { return false; }; - let ty::Param(param) = arg_ty.kind() else { + let ty::Param(param) = *arg_ty.kind() else { return false; }; // Is `generic_param` the same as the arg for this trait predicate? - generic_param.index == generics.type_param(¶m, tcx).index + generic_param.index == generics.type_param(param, tcx).index } else { false } @@ -156,10 +156,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { return false; } - match ty.peel_refs().kind() { + match *ty.peel_refs().kind() { ty::Param(param) => { let generics = self.tcx.generics_of(self.body_id); - let generic_param = generics.type_param(¶m, self.tcx); + let generic_param = generics.type_param(param, self.tcx); for unsatisfied in unsatisfied_predicates.iter() { // The parameter implements `IntoIterator` // but it has called a method that requires it to implement `Iterator` @@ -3232,9 +3232,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { .sort_by_key(|&info| (!info.def_id.is_local(), self.tcx.def_path_str(info.def_id))); candidates.dedup(); - let param_type = match rcvr_ty.kind() { + let param_type = match *rcvr_ty.kind() { ty::Param(param) => Some(param), - ty::Ref(_, ty, _) => match ty.kind() { + ty::Ref(_, ty, _) => match *ty.kind() { ty::Param(param) => Some(param), _ => None, }, @@ -3429,7 +3429,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } else { param_type.map_or_else( || "implement".to_string(), // FIXME: it might only need to be imported into scope, not implemented. - ToString::to_string, + |p| p.to_string(), ) }, }, diff --git a/compiler/rustc_infer/src/infer/error_reporting/mod.rs b/compiler/rustc_infer/src/infer/error_reporting/mod.rs index 29c9f08a1660d..d753b63be2c0e 100644 --- a/compiler/rustc_infer/src/infer/error_reporting/mod.rs +++ b/compiler/rustc_infer/src/infer/error_reporting/mod.rs @@ -2354,7 +2354,7 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> { // type_param_sugg_span is (span, has_bounds) let (type_scope, type_param_sugg_span) = match bound_kind { - GenericKind::Param(ref param) => { + GenericKind::Param(param) => { let generics = self.tcx.generics_of(generic_param_scope); let def_id = generics.type_param(param, self.tcx).def_id.expect_local(); let scope = self.tcx.local_def_id_to_hir_id(def_id).owner.def_id; diff --git a/compiler/rustc_infer/src/infer/error_reporting/note_and_explain.rs b/compiler/rustc_infer/src/infer/error_reporting/note_and_explain.rs index 008b75b4c9a18..6a42f9b42c384 100644 --- a/compiler/rustc_infer/src/infer/error_reporting/note_and_explain.rs +++ b/compiler/rustc_infer/src/infer/error_reporting/note_and_explain.rs @@ -28,7 +28,7 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> { match err { ArgumentSorts(values, _) | Sorts(values) => { - match (values.expected.kind(), values.found.kind()) { + match (*values.expected.kind(), *values.found.kind()) { (ty::Closure(..), ty::Closure(..)) => { diag.note("no two closures, even if identical, have the same type"); diag.help("consider boxing your closure and/or using it as a trait object"); @@ -452,7 +452,7 @@ impl Trait for X { } (ty::FnPtr(sig), ty::FnDef(def_id, _)) | (ty::FnDef(def_id, _), ty::FnPtr(sig)) => { - if tcx.fn_sig(*def_id).skip_binder().unsafety() < sig.unsafety() { + if tcx.fn_sig(def_id).skip_binder().unsafety() < sig.unsafety() { diag.note( "unsafe functions cannot be coerced into safe function pointers", ); @@ -527,7 +527,7 @@ impl Trait for X { diag: &mut Diag<'_>, msg: impl Fn() -> String, body_owner_def_id: DefId, - proj_ty: &ty::AliasTy<'tcx>, + proj_ty: ty::AliasTy<'tcx>, ty: Ty<'tcx>, ) -> bool { let tcx = self.tcx; @@ -541,7 +541,7 @@ impl Trait for X { }; // Get the `DefId` for the type parameter corresponding to `A` in `::Foo`. // This will also work for `impl Trait`. - let ty::Param(param_ty) = proj_ty.self_ty().kind() else { + let ty::Param(param_ty) = *proj_ty.self_ty().kind() else { return false; }; let generics = tcx.generics_of(body_owner_def_id); @@ -598,7 +598,7 @@ impl Trait for X { fn expected_projection( &self, diag: &mut Diag<'_>, - proj_ty: &ty::AliasTy<'tcx>, + proj_ty: ty::AliasTy<'tcx>, values: ExpectedFound>, body_owner_def_id: DefId, cause_code: &ObligationCauseCode<'_>, @@ -709,7 +709,7 @@ fn foo(&self) -> Self::T { String::new() } &self, diag: &mut Diag<'_>, msg: impl Fn() -> String, - proj_ty: &ty::AliasTy<'tcx>, + proj_ty: ty::AliasTy<'tcx>, ty: Ty<'tcx>, ) -> bool { let tcx = self.tcx; diff --git a/compiler/rustc_middle/src/ty/generics.rs b/compiler/rustc_middle/src/ty/generics.rs index e984f54370178..a99196cb3630d 100644 --- a/compiler/rustc_middle/src/ty/generics.rs +++ b/compiler/rustc_middle/src/ty/generics.rs @@ -263,7 +263,7 @@ impl<'tcx> Generics { /// Returns the `GenericParamDef` associated with this `EarlyParamRegion`. pub fn region_param( &'tcx self, - param: &ty::EarlyParamRegion, + param: ty::EarlyParamRegion, tcx: TyCtxt<'tcx>, ) -> &'tcx GenericParamDef { let param = self.param_at(param.index as usize, tcx); @@ -274,7 +274,7 @@ impl<'tcx> Generics { } /// Returns the `GenericParamDef` associated with this `ParamTy`. - pub fn type_param(&'tcx self, param: &ParamTy, tcx: TyCtxt<'tcx>) -> &'tcx GenericParamDef { + pub fn type_param(&'tcx self, param: ParamTy, tcx: TyCtxt<'tcx>) -> &'tcx GenericParamDef { let param = self.param_at(param.index as usize, tcx); match param.kind { GenericParamDefKind::Type { .. } => param, @@ -286,7 +286,7 @@ impl<'tcx> Generics { /// `Generics`. pub fn opt_type_param( &'tcx self, - param: &ParamTy, + param: ParamTy, tcx: TyCtxt<'tcx>, ) -> Option<&'tcx GenericParamDef> { let param = self.opt_param_at(param.index as usize, tcx)?; @@ -297,7 +297,7 @@ impl<'tcx> Generics { } /// Returns the `GenericParamDef` associated with this `ParamConst`. - pub fn const_param(&'tcx self, param: &ParamConst, tcx: TyCtxt<'tcx>) -> &GenericParamDef { + pub fn const_param(&'tcx self, param: ParamConst, tcx: TyCtxt<'tcx>) -> &GenericParamDef { let param = self.param_at(param.index as usize, tcx); match param.kind { GenericParamDefKind::Const { .. } => param, diff --git a/compiler/rustc_middle/src/ty/print/pretty.rs b/compiler/rustc_middle/src/ty/print/pretty.rs index 0bd009cd51da1..e5450182bf299 100644 --- a/compiler/rustc_middle/src/ty/print/pretty.rs +++ b/compiler/rustc_middle/src/ty/print/pretty.rs @@ -1276,7 +1276,7 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write { fn pretty_print_inherent_projection( &mut self, - alias_ty: &ty::AliasTy<'tcx>, + alias_ty: ty::AliasTy<'tcx>, ) -> Result<(), PrintError> { let def_key = self.tcx().def_key(alias_ty.def_id); self.path_generic_args( @@ -3204,7 +3204,7 @@ define_print_and_forward_display! { ty::AliasTy<'tcx> { if let DefKind::Impl { of_trait: false } = cx.tcx().def_kind(cx.tcx().parent(self.def_id)) { - p!(pretty_print_inherent_projection(self)) + p!(pretty_print_inherent_projection(*self)) } else { // If we're printing verbosely, or don't want to invoke queries // (`is_impl_trait_in_trait`), then fall back to printing the def path. diff --git a/compiler/rustc_middle/src/ty/sty.rs b/compiler/rustc_middle/src/ty/sty.rs index 6084e8d7cab2a..2a1af97ef2480 100644 --- a/compiler/rustc_middle/src/ty/sty.rs +++ b/compiler/rustc_middle/src/ty/sty.rs @@ -1379,7 +1379,7 @@ impl<'tcx> ParamTy { Ty::new_param(tcx, self.index, self.name) } - pub fn span_from_generics(&self, tcx: TyCtxt<'tcx>, item_with_generics: DefId) -> Span { + pub fn span_from_generics(self, tcx: TyCtxt<'tcx>, item_with_generics: DefId) -> Span { let generics = tcx.generics_of(item_with_generics); let type_param = generics.type_param(self, tcx); tcx.def_span(type_param.def_id) diff --git a/compiler/rustc_middle/src/ty/util.rs b/compiler/rustc_middle/src/ty/util.rs index 9af665cfb6fe0..793ac126aa653 100644 --- a/compiler/rustc_middle/src/ty/util.rs +++ b/compiler/rustc_middle/src/ty/util.rs @@ -432,19 +432,19 @@ impl<'tcx> TyCtxt<'tcx> { .filter(|&(_, k)| { match k.unpack() { GenericArgKind::Lifetime(region) => match region.kind() { - ty::ReEarlyParam(ref ebr) => { + ty::ReEarlyParam(ebr) => { !impl_generics.region_param(ebr, self).pure_wrt_drop } // Error: not a region param _ => false, }, - GenericArgKind::Type(ty) => match ty.kind() { - ty::Param(ref pt) => !impl_generics.type_param(pt, self).pure_wrt_drop, + GenericArgKind::Type(ty) => match *ty.kind() { + ty::Param(pt) => !impl_generics.type_param(pt, self).pure_wrt_drop, // Error: not a type param _ => false, }, GenericArgKind::Const(ct) => match ct.kind() { - ty::ConstKind::Param(ref pc) => { + ty::ConstKind::Param(pc) => { !impl_generics.const_param(pc, self).pure_wrt_drop } // Error: not a const param diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs index db3794c1c40b7..9d3caaa01abd7 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs @@ -124,7 +124,7 @@ pub fn suggest_restriction<'tcx, G: EmissionGuarantee>( msg: &str, err: &mut Diag<'_, G>, fn_sig: Option<&hir::FnSig<'_>>, - projection: Option<&ty::AliasTy<'_>>, + projection: Option>, trait_pred: ty::PolyTraitPredicate<'tcx>, // When we are dealing with a trait, `super_traits` will be `Some`: // Given `trait T: A + B + C {}` @@ -142,7 +142,7 @@ pub fn suggest_restriction<'tcx, G: EmissionGuarantee>( let generics = tcx.generics_of(item_id); // Given `fn foo(t: impl Trait)` where `Trait` requires assoc type `A`... if let Some((param, bound_str, fn_sig)) = - fn_sig.zip(projection).and_then(|(sig, p)| match p.self_ty().kind() { + fn_sig.zip(projection).and_then(|(sig, p)| match *p.self_ty().kind() { // Shenanigans to get the `Trait` from the `impl Trait`. ty::Param(param) => { let param_def = generics.type_param(param, tcx); @@ -252,7 +252,7 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> { let trait_pred = self.resolve_numeric_literals_with_default(trait_pred); let self_ty = trait_pred.skip_binder().self_ty(); - let (param_ty, projection) = match self_ty.kind() { + let (param_ty, projection) = match *self_ty.kind() { ty::Param(_) => (true, None), ty::Alias(ty::Projection, projection) => (false, Some(projection)), _ => (false, None), diff --git a/compiler/rustc_trait_selection/src/traits/mod.rs b/compiler/rustc_trait_selection/src/traits/mod.rs index 8f5a30c436d33..88ebf8754d3c2 100644 --- a/compiler/rustc_trait_selection/src/traits/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/mod.rs @@ -482,7 +482,7 @@ fn is_impossible_associated_item( type Result = ControlFlow<()>; fn visit_ty(&mut self, t: Ty<'tcx>) -> Self::Result { // If this is a parameter from the trait item's own generics, then bail - if let ty::Param(param) = t.kind() + if let ty::Param(param) = *t.kind() && let param_def_id = self.generics.type_param(param, self.tcx).def_id && self.tcx.parent(param_def_id) == self.trait_item_def_id { @@ -492,7 +492,7 @@ fn is_impossible_associated_item( } fn visit_region(&mut self, r: ty::Region<'tcx>) -> Self::Result { if let ty::ReEarlyParam(param) = r.kind() - && let param_def_id = self.generics.region_param(¶m, self.tcx).def_id + && let param_def_id = self.generics.region_param(param, self.tcx).def_id && self.tcx.parent(param_def_id) == self.trait_item_def_id { return ControlFlow::Break(()); @@ -501,7 +501,7 @@ fn is_impossible_associated_item( } fn visit_const(&mut self, ct: ty::Const<'tcx>) -> Self::Result { if let ty::ConstKind::Param(param) = ct.kind() - && let param_def_id = self.generics.const_param(¶m, self.tcx).def_id + && let param_def_id = self.generics.const_param(param, self.tcx).def_id && self.tcx.parent(param_def_id) == self.trait_item_def_id { return ControlFlow::Break(()); diff --git a/compiler/rustc_ty_utils/src/opaque_types.rs b/compiler/rustc_ty_utils/src/opaque_types.rs index a652bb781161c..d7d31a88c9b51 100644 --- a/compiler/rustc_ty_utils/src/opaque_types.rs +++ b/compiler/rustc_ty_utils/src/opaque_types.rs @@ -130,7 +130,7 @@ impl<'tcx> OpaqueTypeCollector<'tcx> { TaitInBodyFinder { collector: self }.visit_expr(body); } - fn visit_opaque_ty(&mut self, alias_ty: &ty::AliasTy<'tcx>) { + fn visit_opaque_ty(&mut self, alias_ty: ty::AliasTy<'tcx>) { if !self.seen.insert(alias_ty.def_id.expect_local()) { return; } @@ -205,7 +205,7 @@ impl<'tcx> TypeVisitor> for OpaqueTypeCollector<'tcx> { #[instrument(skip(self), ret, level = "trace")] fn visit_ty(&mut self, t: Ty<'tcx>) { t.super_visit_with(self); - match t.kind() { + match *t.kind() { ty::Alias(ty::Opaque, alias_ty) if alias_ty.def_id.is_local() => { self.visit_opaque_ty(alias_ty); } @@ -279,7 +279,7 @@ impl<'tcx> TypeVisitor> for OpaqueTypeCollector<'tcx> { // assumption to the `param_env` of the default method. We also separately // rely on that assumption here. let ty = self.tcx.type_of(alias_ty.def_id).instantiate(self.tcx, alias_ty.args); - let ty::Alias(ty::Opaque, alias_ty) = ty.kind() else { bug!("{ty:?}") }; + let ty::Alias(ty::Opaque, alias_ty) = *ty.kind() else { bug!("{ty:?}") }; self.visit_opaque_ty(alias_ty); } } From 875f0c2da05da9a7620afd6e2be04fbcb9a4b395 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 21 Apr 2024 11:35:02 +0200 Subject: [PATCH 03/13] Miri: detect wrong vtables in wide pointers --- compiler/rustc_const_eval/messages.ftl | 12 +-- compiler/rustc_const_eval/src/errors.rs | 17 ++++ .../rustc_const_eval/src/interpret/cast.rs | 6 +- .../rustc_const_eval/src/interpret/place.rs | 17 +++- .../src/interpret/terminator.rs | 20 ++-- .../src/interpret/validity.rs | 23 ++++- .../rustc_const_eval/src/interpret/visitor.rs | 8 +- .../rustc_middle/src/mir/interpret/error.rs | 99 +++++++++++++++---- .../tests/fail/dyn-call-trait-mismatch.rs | 5 +- .../tests/fail/dyn-call-trait-mismatch.stderr | 4 +- .../tests/fail/dyn-upcast-nop-wrong-trait.rs | 15 +++ .../fail/dyn-upcast-nop-wrong-trait.stderr | 15 +++ .../tests/fail/dyn-upcast-trait-mismatch.rs | 9 +- .../fail/dyn-upcast-trait-mismatch.stderr | 6 +- .../fail/validity/wrong-dyn-trait-generic.rs | 12 +++ .../validity/wrong-dyn-trait-generic.stderr | 15 +++ .../tests/fail/validity/wrong-dyn-trait.rs | 6 ++ .../fail/validity/wrong-dyn-trait.stderr | 15 +++ .../tests/pass/cast-rfc0401-vtable-kinds.rs | 2 +- src/tools/miri/tests/pass/dyn-upcast.rs | 42 ++++---- tests/ui/cast/cast-rfc0401-vtable-kinds.rs | 2 +- 21 files changed, 265 insertions(+), 85 deletions(-) create mode 100644 src/tools/miri/tests/fail/dyn-upcast-nop-wrong-trait.rs create mode 100644 src/tools/miri/tests/fail/dyn-upcast-nop-wrong-trait.stderr create mode 100644 src/tools/miri/tests/fail/validity/wrong-dyn-trait-generic.rs create mode 100644 src/tools/miri/tests/fail/validity/wrong-dyn-trait-generic.stderr create mode 100644 src/tools/miri/tests/fail/validity/wrong-dyn-trait.rs create mode 100644 src/tools/miri/tests/fail/validity/wrong-dyn-trait.stderr diff --git a/compiler/rustc_const_eval/messages.ftl b/compiler/rustc_const_eval/messages.ftl index f6937dc145d5b..b79d7441acac7 100644 --- a/compiler/rustc_const_eval/messages.ftl +++ b/compiler/rustc_const_eval/messages.ftl @@ -82,12 +82,6 @@ const_eval_double_storage_live = const_eval_dyn_call_not_a_method = `dyn` call trying to call something that is not a method -const_eval_dyn_call_vtable_mismatch = - `dyn` call on a pointer whose vtable does not match its type - -const_eval_dyn_star_call_vtable_mismatch = - `dyn*` call on a pointer whose vtable does not match its type - const_eval_error = {$error_kind -> [static] could not evaluate static initializer [const] evaluation of constant value failed @@ -192,6 +186,8 @@ const_eval_invalid_uninit_bytes_unknown = const_eval_invalid_vtable_pointer = using {$pointer} as vtable pointer but it does not point to a vtable +const_eval_invalid_vtable_trait = + using vtable for trait `{$vtable_trait}` but trait `{$expected_trait}` was expected const_eval_live_drop = destructor of `{$dropped_ty}` cannot be evaluated at compile-time @@ -401,9 +397,6 @@ const_eval_unterminated_c_string = const_eval_unwind_past_top = unwinding past the topmost frame of the stack -const_eval_upcast_mismatch = - upcast on a pointer whose vtable does not match its type - ## The `front_matter`s here refer to either `const_eval_front_matter_invalid_value` or `const_eval_front_matter_invalid_value_with_path`. ## (We'd love to sort this differently to make that more clear but tidy won't let us...) const_eval_validation_box_to_static = {$front_matter}: encountered a box pointing to a static variable in a constant @@ -450,6 +443,7 @@ const_eval_validation_invalid_fn_ptr = {$front_matter}: encountered {$value}, bu const_eval_validation_invalid_ref_meta = {$front_matter}: encountered invalid reference metadata: total size is bigger than largest supported object const_eval_validation_invalid_ref_slice_meta = {$front_matter}: encountered invalid reference metadata: slice is bigger than largest supported object const_eval_validation_invalid_vtable_ptr = {$front_matter}: encountered {$value}, but expected a vtable pointer +const_eval_validation_invalid_vtable_trait = {$front_matter}: wrong trait in wide pointer vtable: expected `{$ref_trait}`, but encountered `{$vtable_trait}` const_eval_validation_mutable_ref_to_immutable = {$front_matter}: encountered mutable reference or box pointing to read-only memory const_eval_validation_never_val = {$front_matter}: encountered a value of the never type `!` const_eval_validation_null_box = {$front_matter}: encountered a null box diff --git a/compiler/rustc_const_eval/src/errors.rs b/compiler/rustc_const_eval/src/errors.rs index a60cedd6500d5..90d4f1168e4fd 100644 --- a/compiler/rustc_const_eval/src/errors.rs +++ b/compiler/rustc_const_eval/src/errors.rs @@ -498,6 +498,7 @@ impl<'a> ReportErrorExt for UndefinedBehaviorInfo<'a> { InvalidTag(_) => const_eval_invalid_tag, InvalidFunctionPointer(_) => const_eval_invalid_function_pointer, InvalidVTablePointer(_) => const_eval_invalid_vtable_pointer, + InvalidVTableTrait { .. } => const_eval_invalid_vtable_trait, InvalidStr(_) => const_eval_invalid_str, InvalidUninitBytes(None) => const_eval_invalid_uninit_bytes_unknown, InvalidUninitBytes(Some(_)) => const_eval_invalid_uninit_bytes, @@ -537,6 +538,7 @@ impl<'a> ReportErrorExt for UndefinedBehaviorInfo<'a> { | DeadLocal | UninhabitedEnumVariantWritten(_) | UninhabitedEnumVariantRead(_) => {} + BoundsCheckFailed { len, index } => { diag.arg("len", len); diag.arg("index", index); @@ -544,6 +546,13 @@ impl<'a> ReportErrorExt for UndefinedBehaviorInfo<'a> { UnterminatedCString(ptr) | InvalidFunctionPointer(ptr) | InvalidVTablePointer(ptr) => { diag.arg("pointer", ptr); } + InvalidVTableTrait { expected_trait, vtable_trait } => { + diag.arg("expected_trait", expected_trait.to_string()); + diag.arg( + "vtable_trait", + vtable_trait.map(|t| t.to_string()).unwrap_or_else(|| format!("")), + ); + } PointerUseAfterFree(alloc_id, msg) => { diag.arg("alloc_id", alloc_id) .arg("bad_pointer_message", bad_pointer_message(msg, dcx)); @@ -634,6 +643,7 @@ impl<'tcx> ReportErrorExt for ValidationErrorInfo<'tcx> { UninhabitedEnumVariant => const_eval_validation_uninhabited_enum_variant, Uninit { .. } => const_eval_validation_uninit, InvalidVTablePtr { .. } => const_eval_validation_invalid_vtable_ptr, + InvalidMetaWrongTrait { .. } => const_eval_validation_invalid_vtable_trait, InvalidMetaSliceTooLarge { ptr_kind: PointerKind::Box } => { const_eval_validation_invalid_box_slice_meta } @@ -773,6 +783,13 @@ impl<'tcx> ReportErrorExt for ValidationErrorInfo<'tcx> { DanglingPtrNoProvenance { pointer, .. } => { err.arg("pointer", pointer); } + InvalidMetaWrongTrait { expected_trait: ref_trait, vtable_trait } => { + err.arg("ref_trait", ref_trait.to_string()); + err.arg( + "vtable_trait", + vtable_trait.map(|t| t.to_string()).unwrap_or_else(|| format!("")), + ); + } NullPtr { .. } | PtrToStatic { .. } | ConstRefToMutable diff --git a/compiler/rustc_const_eval/src/interpret/cast.rs b/compiler/rustc_const_eval/src/interpret/cast.rs index 9447d18fe8c93..76e59ea90559f 100644 --- a/compiler/rustc_const_eval/src/interpret/cast.rs +++ b/compiler/rustc_const_eval/src/interpret/cast.rs @@ -393,6 +393,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let val = self.read_immediate(src)?; if data_a.principal() == data_b.principal() { // A NOP cast that doesn't actually change anything, should be allowed even with mismatching vtables. + // (But currently mismatching vtables violate the validity invariant so UB is triggered anyway.) return self.write_immediate(*val, dest); } let (old_data, old_vptr) = val.to_scalar_pair(); @@ -400,7 +401,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let old_vptr = old_vptr.to_pointer(self)?; let (ty, old_trait) = self.get_ptr_vtable(old_vptr)?; if old_trait != data_a.principal() { - throw_ub_custom!(fluent::const_eval_upcast_mismatch); + throw_ub!(InvalidVTableTrait { + expected_trait: data_a, + vtable_trait: old_trait, + }); } let new_vptr = self.get_vtable_ptr(ty, data_b.principal())?; self.write_immediate(Immediate::new_dyn_trait(old_data, new_vptr, self), dest) diff --git a/compiler/rustc_const_eval/src/interpret/place.rs b/compiler/rustc_const_eval/src/interpret/place.rs index 8364a5a8d186a..e5241f1ba19e1 100644 --- a/compiler/rustc_const_eval/src/interpret/place.rs +++ b/compiler/rustc_const_eval/src/interpret/place.rs @@ -1020,16 +1020,20 @@ where pub(super) fn unpack_dyn_trait( &self, mplace: &MPlaceTy<'tcx, M::Provenance>, + expected_trait: &'tcx ty::List>, ) -> InterpResult<'tcx, (MPlaceTy<'tcx, M::Provenance>, Pointer>)> { assert!( matches!(mplace.layout.ty.kind(), ty::Dynamic(_, _, ty::Dyn)), "`unpack_dyn_trait` only makes sense on `dyn*` types" ); let vtable = mplace.meta().unwrap_meta().to_pointer(self)?; - let (ty, _) = self.get_ptr_vtable(vtable)?; - let layout = self.layout_of(ty)?; + let (ty, vtable_trait) = self.get_ptr_vtable(vtable)?; + if expected_trait.principal() != vtable_trait { + throw_ub!(InvalidVTableTrait { expected_trait, vtable_trait }); + } // This is a kind of transmute, from a place with unsized type and metadata to // a place with sized type and no metadata. + let layout = self.layout_of(ty)?; let mplace = MPlaceTy { mplace: MemPlace { meta: MemPlaceMeta::None, ..mplace.mplace }, layout }; Ok((mplace, vtable)) @@ -1040,6 +1044,7 @@ where pub(super) fn unpack_dyn_star>( &self, val: &P, + expected_trait: &'tcx ty::List>, ) -> InterpResult<'tcx, (P, Pointer>)> { assert!( matches!(val.layout().ty.kind(), ty::Dynamic(_, _, ty::DynStar)), @@ -1048,10 +1053,12 @@ where let data = self.project_field(val, 0)?; let vtable = self.project_field(val, 1)?; let vtable = self.read_pointer(&vtable.to_op(self)?)?; - let (ty, _) = self.get_ptr_vtable(vtable)?; + let (ty, vtable_trait) = self.get_ptr_vtable(vtable)?; + if expected_trait.principal() != vtable_trait { + throw_ub!(InvalidVTableTrait { expected_trait, vtable_trait }); + } + // `data` is already the right thing but has the wrong type. So we transmute it. let layout = self.layout_of(ty)?; - // `data` is already the right thing but has the wrong type. So we transmute it, by - // projecting with offset 0. let data = data.transmute(layout, self)?; Ok((data, vtable)) } diff --git a/compiler/rustc_const_eval/src/interpret/terminator.rs b/compiler/rustc_const_eval/src/interpret/terminator.rs index c0e27e86d500a..0c3b01d449bd8 100644 --- a/compiler/rustc_const_eval/src/interpret/terminator.rs +++ b/compiler/rustc_const_eval/src/interpret/terminator.rs @@ -802,11 +802,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let (vptr, dyn_ty, adjusted_receiver) = if let ty::Dynamic(data, _, ty::DynStar) = receiver_place.layout.ty.kind() { - let (recv, vptr) = self.unpack_dyn_star(&receiver_place)?; - let (dyn_ty, dyn_trait) = self.get_ptr_vtable(vptr)?; - if dyn_trait != data.principal() { - throw_ub_custom!(fluent::const_eval_dyn_star_call_vtable_mismatch); - } + let (recv, vptr) = self.unpack_dyn_star(&receiver_place, data)?; + let (dyn_ty, _dyn_trait) = self.get_ptr_vtable(vptr)?; (vptr, dyn_ty, recv.ptr()) } else { @@ -828,7 +825,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let vptr = receiver_place.meta().unwrap_meta().to_pointer(self)?; let (dyn_ty, dyn_trait) = self.get_ptr_vtable(vptr)?; if dyn_trait != data.principal() { - throw_ub_custom!(fluent::const_eval_dyn_call_vtable_mismatch); + throw_ub!(InvalidVTableTrait { + expected_trait: data, + vtable_trait: dyn_trait, + }); } // It might be surprising that we use a pointer as the receiver even if this @@ -938,13 +938,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let place = self.force_allocation(place)?; let place = match place.layout.ty.kind() { - ty::Dynamic(_, _, ty::Dyn) => { + ty::Dynamic(data, _, ty::Dyn) => { // Dropping a trait object. Need to find actual drop fn. - self.unpack_dyn_trait(&place)?.0 + self.unpack_dyn_trait(&place, data)?.0 } - ty::Dynamic(_, _, ty::DynStar) => { + ty::Dynamic(data, _, ty::DynStar) => { // Dropping a `dyn*`. Need to find actual drop fn. - self.unpack_dyn_star(&place)?.0 + self.unpack_dyn_star(&place, data)?.0 } _ => { debug_assert_eq!( diff --git a/compiler/rustc_const_eval/src/interpret/validity.rs b/compiler/rustc_const_eval/src/interpret/validity.rs index b8a1733e45ad6..14566719ccd72 100644 --- a/compiler/rustc_const_eval/src/interpret/validity.rs +++ b/compiler/rustc_const_eval/src/interpret/validity.rs @@ -339,16 +339,22 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' ) -> InterpResult<'tcx> { let tail = self.ecx.tcx.struct_tail_erasing_lifetimes(pointee.ty, self.ecx.param_env); match tail.kind() { - ty::Dynamic(_, _, ty::Dyn) => { + ty::Dynamic(data, _, ty::Dyn) => { let vtable = meta.unwrap_meta().to_pointer(self.ecx)?; // Make sure it is a genuine vtable pointer. - let (_ty, _trait) = try_validation!( + let (_dyn_ty, dyn_trait) = try_validation!( self.ecx.get_ptr_vtable(vtable), self.path, Ub(DanglingIntPointer(..) | InvalidVTablePointer(..)) => InvalidVTablePtr { value: format!("{vtable}") } ); - // FIXME: check if the type/trait match what ty::Dynamic says? + // Make sure it is for the right trait. + if dyn_trait != data.principal() { + throw_validation_failure!( + self.path, + InvalidMetaWrongTrait { expected_trait: data, vtable_trait: dyn_trait } + ); + } } ty::Slice(..) | ty::Str => { let _len = meta.unwrap_meta().to_target_usize(self.ecx)?; @@ -933,7 +939,16 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> } } _ => { - self.walk_value(op)?; // default handler + // default handler + try_validation!( + self.walk_value(op), + self.path, + // It's not great to catch errors here, since we can't give a very good path, + // but it's better than ICEing. + Ub(InvalidVTableTrait { expected_trait, vtable_trait }) => { + InvalidMetaWrongTrait { expected_trait, vtable_trait: *vtable_trait } + }, + ); } } diff --git a/compiler/rustc_const_eval/src/interpret/visitor.rs b/compiler/rustc_const_eval/src/interpret/visitor.rs index 0e824f3f592d4..84557b8e2d600 100644 --- a/compiler/rustc_const_eval/src/interpret/visitor.rs +++ b/compiler/rustc_const_eval/src/interpret/visitor.rs @@ -88,22 +88,22 @@ pub trait ValueVisitor<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>>: Sized { // Special treatment for special types, where the (static) layout is not sufficient. match *ty.kind() { // If it is a trait object, switch to the real type that was used to create it. - ty::Dynamic(_, _, ty::Dyn) => { + ty::Dynamic(data, _, ty::Dyn) => { // Dyn types. This is unsized, and the actual dynamic type of the data is given by the // vtable stored in the place metadata. // unsized values are never immediate, so we can assert_mem_place let op = v.to_op(self.ecx())?; let dest = op.assert_mem_place(); - let inner_mplace = self.ecx().unpack_dyn_trait(&dest)?.0; + let inner_mplace = self.ecx().unpack_dyn_trait(&dest, data)?.0; trace!("walk_value: dyn object layout: {:#?}", inner_mplace.layout); // recurse with the inner type return self.visit_field(v, 0, &inner_mplace.into()); } - ty::Dynamic(_, _, ty::DynStar) => { + ty::Dynamic(data, _, ty::DynStar) => { // DynStar types. Very different from a dyn type (but strangely part of the // same variant in `TyKind`): These are pairs where the 2nd component is the // vtable, and the first component is the data (which must be ptr-sized). - let data = self.ecx().unpack_dyn_star(v)?.0; + let data = self.ecx().unpack_dyn_star(v, data)?.0; return self.visit_field(v, 0, &data); } // Slices do not need special handling here: they have `Array` field diff --git a/compiler/rustc_middle/src/mir/interpret/error.rs b/compiler/rustc_middle/src/mir/interpret/error.rs index 65ce1cd8f50c6..a3d16d4f097a8 100644 --- a/compiler/rustc_middle/src/mir/interpret/error.rs +++ b/compiler/rustc_middle/src/mir/interpret/error.rs @@ -2,7 +2,7 @@ use super::{AllocId, AllocRange, ConstAllocation, Pointer, Scalar}; use crate::error; use crate::mir::{ConstAlloc, ConstValue}; -use crate::ty::{layout, tls, Ty, TyCtxt, ValTree}; +use crate::ty::{self, layout, tls, Ty, TyCtxt, ValTree}; use rustc_ast_ir::Mutability; use rustc_data_structures::sync::Lock; @@ -344,6 +344,11 @@ pub enum UndefinedBehaviorInfo<'tcx> { InvalidFunctionPointer(Pointer), /// Using a pointer-not-to-a-vtable as vtable pointer. InvalidVTablePointer(Pointer), + /// Using a vtable for the wrong trait. + InvalidVTableTrait { + expected_trait: &'tcx ty::List>, + vtable_trait: Option>, + }, /// Using a string that is not valid UTF-8, InvalidStr(std::str::Utf8Error), /// Using uninitialized data where it is not allowed. @@ -414,34 +419,86 @@ impl From for ExpectedKind { #[derive(Debug)] pub enum ValidationErrorKind<'tcx> { - PointerAsInt { expected: ExpectedKind }, + PointerAsInt { + expected: ExpectedKind, + }, PartialPointer, - PtrToUninhabited { ptr_kind: PointerKind, ty: Ty<'tcx> }, - PtrToStatic { ptr_kind: PointerKind }, + PtrToUninhabited { + ptr_kind: PointerKind, + ty: Ty<'tcx>, + }, + PtrToStatic { + ptr_kind: PointerKind, + }, ConstRefToMutable, ConstRefToExtern, MutableRefToImmutable, UnsafeCellInImmutable, NullFnPtr, NeverVal, - NullablePtrOutOfRange { range: WrappingRange, max_value: u128 }, - PtrOutOfRange { range: WrappingRange, max_value: u128 }, - OutOfRange { value: String, range: WrappingRange, max_value: u128 }, - UninhabitedVal { ty: Ty<'tcx> }, - InvalidEnumTag { value: String }, + NullablePtrOutOfRange { + range: WrappingRange, + max_value: u128, + }, + PtrOutOfRange { + range: WrappingRange, + max_value: u128, + }, + OutOfRange { + value: String, + range: WrappingRange, + max_value: u128, + }, + UninhabitedVal { + ty: Ty<'tcx>, + }, + InvalidEnumTag { + value: String, + }, UninhabitedEnumVariant, - Uninit { expected: ExpectedKind }, - InvalidVTablePtr { value: String }, - InvalidMetaSliceTooLarge { ptr_kind: PointerKind }, - InvalidMetaTooLarge { ptr_kind: PointerKind }, - UnalignedPtr { ptr_kind: PointerKind, required_bytes: u64, found_bytes: u64 }, - NullPtr { ptr_kind: PointerKind }, - DanglingPtrNoProvenance { ptr_kind: PointerKind, pointer: String }, - DanglingPtrOutOfBounds { ptr_kind: PointerKind }, - DanglingPtrUseAfterFree { ptr_kind: PointerKind }, - InvalidBool { value: String }, - InvalidChar { value: String }, - InvalidFnPtr { value: String }, + Uninit { + expected: ExpectedKind, + }, + InvalidVTablePtr { + value: String, + }, + InvalidMetaWrongTrait { + expected_trait: &'tcx ty::List>, + vtable_trait: Option>, + }, + InvalidMetaSliceTooLarge { + ptr_kind: PointerKind, + }, + InvalidMetaTooLarge { + ptr_kind: PointerKind, + }, + UnalignedPtr { + ptr_kind: PointerKind, + required_bytes: u64, + found_bytes: u64, + }, + NullPtr { + ptr_kind: PointerKind, + }, + DanglingPtrNoProvenance { + ptr_kind: PointerKind, + pointer: String, + }, + DanglingPtrOutOfBounds { + ptr_kind: PointerKind, + }, + DanglingPtrUseAfterFree { + ptr_kind: PointerKind, + }, + InvalidBool { + value: String, + }, + InvalidChar { + value: String, + }, + InvalidFnPtr { + value: String, + }, } /// Error information for when the program did something that might (or might not) be correct diff --git a/src/tools/miri/tests/fail/dyn-call-trait-mismatch.rs b/src/tools/miri/tests/fail/dyn-call-trait-mismatch.rs index 13f913454dcdf..f71df9a1c909d 100644 --- a/src/tools/miri/tests/fail/dyn-call-trait-mismatch.rs +++ b/src/tools/miri/tests/fail/dyn-call-trait-mismatch.rs @@ -1,3 +1,6 @@ +// Validation stops this too early. +//@compile-flags: -Zmiri-disable-validation + trait T1 { #[allow(dead_code)] fn method1(self: Box); @@ -13,5 +16,5 @@ impl T1 for i32 { fn main() { let r = Box::new(0) as Box; let r2: Box = unsafe { std::mem::transmute(r) }; - r2.method2(); //~ERROR: call on a pointer whose vtable does not match its type + r2.method2(); //~ERROR: using vtable for trait `T1` but trait `T2` was expected } diff --git a/src/tools/miri/tests/fail/dyn-call-trait-mismatch.stderr b/src/tools/miri/tests/fail/dyn-call-trait-mismatch.stderr index 365186bcc4b41..019a55bcdcb1a 100644 --- a/src/tools/miri/tests/fail/dyn-call-trait-mismatch.stderr +++ b/src/tools/miri/tests/fail/dyn-call-trait-mismatch.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: `dyn` call on a pointer whose vtable does not match its type +error: Undefined Behavior: using vtable for trait `T1` but trait `T2` was expected --> $DIR/dyn-call-trait-mismatch.rs:LL:CC | LL | r2.method2(); - | ^^^^^^^^^^^^ `dyn` call on a pointer whose vtable does not match its type + | ^^^^^^^^^^^^ using vtable for trait `T1` but trait `T2` was expected | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information diff --git a/src/tools/miri/tests/fail/dyn-upcast-nop-wrong-trait.rs b/src/tools/miri/tests/fail/dyn-upcast-nop-wrong-trait.rs new file mode 100644 index 0000000000000..dff5a21c4c777 --- /dev/null +++ b/src/tools/miri/tests/fail/dyn-upcast-nop-wrong-trait.rs @@ -0,0 +1,15 @@ +// This upcast is currently forbidden because it involves an invalid value. +// However, if in the future we relax the validity requirements for raw pointer vtables, +// we could consider allowing this again -- the cast itself isn't doing anything wrong, +// only the transmutes needed to set up the testcase are wrong. + +use std::fmt; + +fn main() { + // vtable_mismatch_nop_cast + let ptr: &dyn fmt::Display = &0; + let ptr: *const (dyn fmt::Debug + Send + Sync) = unsafe { std::mem::transmute(ptr) }; //~ERROR: wrong trait + // Even though the vtable is for the wrong trait, this cast doesn't actually change the needed + // vtable so it should still be allowed -- if we ever allow the line above. + let _ptr2 = ptr as *const dyn fmt::Debug; +} diff --git a/src/tools/miri/tests/fail/dyn-upcast-nop-wrong-trait.stderr b/src/tools/miri/tests/fail/dyn-upcast-nop-wrong-trait.stderr new file mode 100644 index 0000000000000..4165d5ea15d97 --- /dev/null +++ b/src/tools/miri/tests/fail/dyn-upcast-nop-wrong-trait.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: constructing invalid value: wrong trait in wide pointer vtable: expected `std::fmt::Debug + std::marker::Send + std::marker::Sync`, but encountered `std::fmt::Display` + --> $DIR/dyn-upcast-nop-wrong-trait.rs:LL:CC + | +LL | let ptr: *const (dyn fmt::Debug + Send + Sync) = unsafe { std::mem::transmute(ptr) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value: wrong trait in wide pointer vtable: expected `std::fmt::Debug + std::marker::Send + std::marker::Sync`, but encountered `std::fmt::Display` + | + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE: + = note: inside `main` at $DIR/dyn-upcast-nop-wrong-trait.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + diff --git a/src/tools/miri/tests/fail/dyn-upcast-trait-mismatch.rs b/src/tools/miri/tests/fail/dyn-upcast-trait-mismatch.rs index 982f33b0a3102..1d6b6777032de 100644 --- a/src/tools/miri/tests/fail/dyn-upcast-trait-mismatch.rs +++ b/src/tools/miri/tests/fail/dyn-upcast-trait-mismatch.rs @@ -1,3 +1,6 @@ +// Validation stops this too early. +//@compile-flags: -Zmiri-disable-validation + #![feature(trait_upcasting)] #![allow(incomplete_features)] @@ -57,7 +60,7 @@ impl Baz for i32 { fn main() { let baz: &dyn Baz = &1; - let baz_fake: &dyn Bar = unsafe { std::mem::transmute(baz) }; - let _err = baz_fake as &dyn Foo; - //~^ERROR: upcast on a pointer whose vtable does not match its type + let baz_fake: *const dyn Bar = unsafe { std::mem::transmute(baz) }; + let _err = baz_fake as *const dyn Foo; + //~^ERROR: using vtable for trait `Baz` but trait `Bar` was expected } diff --git a/src/tools/miri/tests/fail/dyn-upcast-trait-mismatch.stderr b/src/tools/miri/tests/fail/dyn-upcast-trait-mismatch.stderr index 8bac908b864bc..6a2415cf57e29 100644 --- a/src/tools/miri/tests/fail/dyn-upcast-trait-mismatch.stderr +++ b/src/tools/miri/tests/fail/dyn-upcast-trait-mismatch.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: upcast on a pointer whose vtable does not match its type +error: Undefined Behavior: using vtable for trait `Baz` but trait `Bar` was expected --> $DIR/dyn-upcast-trait-mismatch.rs:LL:CC | -LL | let _err = baz_fake as &dyn Foo; - | ^^^^^^^^ upcast on a pointer whose vtable does not match its type +LL | let _err = baz_fake as *const dyn Foo; + | ^^^^^^^^ using vtable for trait `Baz` but trait `Bar` was expected | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information diff --git a/src/tools/miri/tests/fail/validity/wrong-dyn-trait-generic.rs b/src/tools/miri/tests/fail/validity/wrong-dyn-trait-generic.rs new file mode 100644 index 0000000000000..9b1cefc4b1d3d --- /dev/null +++ b/src/tools/miri/tests/fail/validity/wrong-dyn-trait-generic.rs @@ -0,0 +1,12 @@ +use std::mem; + +// Make sure we notice the mismatch also if the difference is "only" in the generic +// parameters of the trait. + +trait Trait {} +impl Trait for T {} + +fn main() { + let x: &dyn Trait = &0; + let _y: *const dyn Trait = unsafe { mem::transmute(x) }; //~ERROR: wrong trait +} diff --git a/src/tools/miri/tests/fail/validity/wrong-dyn-trait-generic.stderr b/src/tools/miri/tests/fail/validity/wrong-dyn-trait-generic.stderr new file mode 100644 index 0000000000000..1219f9f88cf8c --- /dev/null +++ b/src/tools/miri/tests/fail/validity/wrong-dyn-trait-generic.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: constructing invalid value: wrong trait in wide pointer vtable: expected `Trait`, but encountered `Trait` + --> $DIR/wrong-dyn-trait-generic.rs:LL:CC + | +LL | let _y: *const dyn Trait = unsafe { mem::transmute(x) }; + | ^^^^^^^^^^^^^^^^^ constructing invalid value: wrong trait in wide pointer vtable: expected `Trait`, but encountered `Trait` + | + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE: + = note: inside `main` at $DIR/wrong-dyn-trait-generic.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + diff --git a/src/tools/miri/tests/fail/validity/wrong-dyn-trait.rs b/src/tools/miri/tests/fail/validity/wrong-dyn-trait.rs new file mode 100644 index 0000000000000..d6049196f2662 --- /dev/null +++ b/src/tools/miri/tests/fail/validity/wrong-dyn-trait.rs @@ -0,0 +1,6 @@ +use std::{fmt, mem}; + +fn main() { + let x: &dyn Send = &0; + let _y: *const dyn fmt::Debug = unsafe { mem::transmute(x) }; //~ERROR: wrong trait +} diff --git a/src/tools/miri/tests/fail/validity/wrong-dyn-trait.stderr b/src/tools/miri/tests/fail/validity/wrong-dyn-trait.stderr new file mode 100644 index 0000000000000..e3503323b31a7 --- /dev/null +++ b/src/tools/miri/tests/fail/validity/wrong-dyn-trait.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: constructing invalid value: wrong trait in wide pointer vtable: expected `std::fmt::Debug`, but encountered `` + --> $DIR/wrong-dyn-trait.rs:LL:CC + | +LL | let _y: *const dyn fmt::Debug = unsafe { mem::transmute(x) }; + | ^^^^^^^^^^^^^^^^^ constructing invalid value: wrong trait in wide pointer vtable: expected `std::fmt::Debug`, but encountered `` + | + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE: + = note: inside `main` at $DIR/wrong-dyn-trait.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + diff --git a/src/tools/miri/tests/pass/cast-rfc0401-vtable-kinds.rs b/src/tools/miri/tests/pass/cast-rfc0401-vtable-kinds.rs index 2491bda0917d3..adbf5df62cc5d 100644 --- a/src/tools/miri/tests/pass/cast-rfc0401-vtable-kinds.rs +++ b/src/tools/miri/tests/pass/cast-rfc0401-vtable-kinds.rs @@ -25,7 +25,7 @@ impl Foo for u32 { impl Bar for () {} unsafe fn round_trip_and_call<'a>(t: *const (dyn Foo + 'a)) -> u32 { - let foo_e: *const dyn Foo = t as *const _; + let foo_e: *const dyn Foo = t as *const _; let r_1 = foo_e as *mut dyn Foo; (&*r_1).foo(0) diff --git a/src/tools/miri/tests/pass/dyn-upcast.rs b/src/tools/miri/tests/pass/dyn-upcast.rs index 529b9c471d426..ddc4bdcf082a8 100644 --- a/src/tools/miri/tests/pass/dyn-upcast.rs +++ b/src/tools/miri/tests/pass/dyn-upcast.rs @@ -1,24 +1,26 @@ #![feature(trait_upcasting)] #![allow(incomplete_features)] +use std::fmt; + fn main() { basic(); diamond(); struct_(); replace_vptr(); - vtable_mismatch_nop_cast(); + vtable_nop_cast(); } -fn vtable_mismatch_nop_cast() { - let ptr: &dyn std::fmt::Display = &0; - // Even though the vtable is for the wrong trait, this cast doesn't actually change the needed - // vtable so it should still be allowed. - let ptr: *const (dyn std::fmt::Debug + Send + Sync) = unsafe { std::mem::transmute(ptr) }; - let _ptr2 = ptr as *const dyn std::fmt::Debug; +fn vtable_nop_cast() { + let ptr: &dyn fmt::Debug = &0; + // We transmute things around, but the principal trait does not change, so this is allowed. + let ptr: *const (dyn fmt::Debug + Send + Sync) = unsafe { std::mem::transmute(ptr) }; + // This cast is a NOP and should be allowed. + let _ptr2 = ptr as *const dyn fmt::Debug; } fn basic() { - trait Foo: PartialEq + std::fmt::Debug + Send + Sync { + trait Foo: PartialEq + fmt::Debug + Send + Sync { fn a(&self) -> i32 { 10 } @@ -67,7 +69,7 @@ fn basic() { } let baz: &dyn Baz = &1; - let _: &dyn std::fmt::Debug = baz; + let _: &dyn fmt::Debug = baz; assert_eq!(*baz, 1); assert_eq!(baz.a(), 100); assert_eq!(baz.b(), 200); @@ -77,7 +79,7 @@ fn basic() { assert_eq!(baz.w(), 21); let bar: &dyn Bar = baz; - let _: &dyn std::fmt::Debug = bar; + let _: &dyn fmt::Debug = bar; assert_eq!(*bar, 1); assert_eq!(bar.a(), 100); assert_eq!(bar.b(), 200); @@ -86,14 +88,14 @@ fn basic() { assert_eq!(bar.w(), 21); let foo: &dyn Foo = baz; - let _: &dyn std::fmt::Debug = foo; + let _: &dyn fmt::Debug = foo; assert_eq!(*foo, 1); assert_eq!(foo.a(), 100); assert_eq!(foo.z(), 11); assert_eq!(foo.y(), 12); let foo: &dyn Foo = bar; - let _: &dyn std::fmt::Debug = foo; + let _: &dyn fmt::Debug = foo; assert_eq!(*foo, 1); assert_eq!(foo.a(), 100); assert_eq!(foo.z(), 11); @@ -101,7 +103,7 @@ fn basic() { } fn diamond() { - trait Foo: PartialEq + std::fmt::Debug + Send + Sync { + trait Foo: PartialEq + fmt::Debug + Send + Sync { fn a(&self) -> i32 { 10 } @@ -166,7 +168,7 @@ fn diamond() { } let baz: &dyn Baz = &1; - let _: &dyn std::fmt::Debug = baz; + let _: &dyn fmt::Debug = baz; assert_eq!(*baz, 1); assert_eq!(baz.a(), 100); assert_eq!(baz.b(), 200); @@ -178,7 +180,7 @@ fn diamond() { assert_eq!(baz.v(), 31); let bar1: &dyn Bar1 = baz; - let _: &dyn std::fmt::Debug = bar1; + let _: &dyn fmt::Debug = bar1; assert_eq!(*bar1, 1); assert_eq!(bar1.a(), 100); assert_eq!(bar1.b(), 200); @@ -187,7 +189,7 @@ fn diamond() { assert_eq!(bar1.w(), 21); let bar2: &dyn Bar2 = baz; - let _: &dyn std::fmt::Debug = bar2; + let _: &dyn fmt::Debug = bar2; assert_eq!(*bar2, 1); assert_eq!(bar2.a(), 100); assert_eq!(bar2.c(), 300); @@ -196,17 +198,17 @@ fn diamond() { assert_eq!(bar2.v(), 31); let foo: &dyn Foo = baz; - let _: &dyn std::fmt::Debug = foo; + let _: &dyn fmt::Debug = foo; assert_eq!(*foo, 1); assert_eq!(foo.a(), 100); let foo: &dyn Foo = bar1; - let _: &dyn std::fmt::Debug = foo; + let _: &dyn fmt::Debug = foo; assert_eq!(*foo, 1); assert_eq!(foo.a(), 100); let foo: &dyn Foo = bar2; - let _: &dyn std::fmt::Debug = foo; + let _: &dyn fmt::Debug = foo; assert_eq!(*foo, 1); assert_eq!(foo.a(), 100); } @@ -215,7 +217,7 @@ fn struct_() { use std::rc::Rc; use std::sync::Arc; - trait Foo: PartialEq + std::fmt::Debug + Send + Sync { + trait Foo: PartialEq + fmt::Debug + Send + Sync { fn a(&self) -> i32 { 10 } diff --git a/tests/ui/cast/cast-rfc0401-vtable-kinds.rs b/tests/ui/cast/cast-rfc0401-vtable-kinds.rs index 410e15d024fab..0d8f92f013fba 100644 --- a/tests/ui/cast/cast-rfc0401-vtable-kinds.rs +++ b/tests/ui/cast/cast-rfc0401-vtable-kinds.rs @@ -17,7 +17,7 @@ impl Foo for u32 { fn foo(&self, _: u32) -> u32 { self+43 } } impl Bar for () {} unsafe fn round_trip_and_call<'a>(t: *const (dyn Foo+'a)) -> u32 { - let foo_e : *const dyn Foo = t as *const _; + let foo_e : *const dyn Foo = t as *const _; let r_1 = foo_e as *mut dyn Foo; (&*r_1).foo(0) From c373ec07c42f9c403c3eed5de431907e7ccbd49d Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Mon, 22 Apr 2024 12:11:07 +0200 Subject: [PATCH 04/13] Improve ICE message for forbidden dep-graph reads. --- .../rustc_query_system/src/dep_graph/graph.rs | 41 ++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index 534937003ebfd..af70e2a426423 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -459,7 +459,8 @@ impl DepGraph { } TaskDepsRef::Ignore => return, TaskDepsRef::Forbid => { - panic!("Illegal read of: {dep_node_index:?}") + // Reading is forbidden in this context. ICE with a useful error message. + panic_on_forbidden_read(data, dep_node_index) } }; let task_deps = &mut *task_deps; @@ -1366,3 +1367,41 @@ pub(crate) fn print_markframe_trace(graph: &DepGraph, frame: Option< eprintln!("end of try_mark_green dep node stack"); } + +#[cold] +#[inline(never)] +fn panic_on_forbidden_read(data: &DepGraphData, dep_node_index: DepNodeIndex) -> ! { + // We have to do an expensive reverse-lookup of the DepNode that + // corresponds to `dep_node_index`, but that's OK since we are about + // to ICE anyway. + let mut dep_node = None; + + // First try to find the dep node among those that already existed in the + // previous session + for (prev_index, index) in data.current.prev_index_to_index.lock().iter_enumerated() { + if index == &Some(dep_node_index) { + dep_node = Some(data.previous.index_to_node(prev_index)); + break; + } + } + + if dep_node.is_none() { + // Try to find it among the new nodes + for shard in data.current.new_node_to_index.lock_shards() { + if let Some((node, _)) = shard.iter().find(|(_, index)| **index == dep_node_index) { + dep_node = Some(*node); + break; + } + } + } + + let dep_node = dep_node.map_or_else( + || format!("with index {:?}", dep_node_index), + |dep_node| format!("`{:?}`", dep_node), + ); + + panic!( + "Error: trying to record dependency on DepNode {dep_node} in a \ + context that does not allow it (e.g. during query deserialization)." + ) +} From 4f7a47798eb3455aed74fde5cd8e81927d2db07a Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 17 Apr 2024 11:41:40 +1000 Subject: [PATCH 05/13] coverage: Branch coverage test for let-else --- tests/coverage/branch/let-else.cov-map | 18 ++++++++++++ tests/coverage/branch/let-else.coverage | 37 +++++++++++++++++++++++++ tests/coverage/branch/let-else.rs | 35 +++++++++++++++++++++++ 3 files changed, 90 insertions(+) create mode 100644 tests/coverage/branch/let-else.cov-map create mode 100644 tests/coverage/branch/let-else.coverage create mode 100644 tests/coverage/branch/let-else.rs diff --git a/tests/coverage/branch/let-else.cov-map b/tests/coverage/branch/let-else.cov-map new file mode 100644 index 0000000000000..ad987bd6bb1e9 --- /dev/null +++ b/tests/coverage/branch/let-else.cov-map @@ -0,0 +1,18 @@ +Function name: let_else::let_else +Raw bytes (38): 0x[01, 01, 02, 05, 09, 09, 02, 06, 01, 0c, 01, 01, 10, 02, 03, 0e, 00, 0f, 05, 00, 13, 00, 18, 09, 01, 09, 01, 0f, 02, 04, 05, 00, 0b, 07, 01, 01, 00, 02] +Number of files: 1 +- file 0 => global file 1 +Number of expressions: 2 +- expression 0 operands: lhs = Counter(1), rhs = Counter(2) +- expression 1 operands: lhs = Counter(2), rhs = Expression(0, Sub) +Number of file 0 mappings: 6 +- Code(Counter(0)) at (prev + 12, 1) to (start + 1, 16) +- Code(Expression(0, Sub)) at (prev + 3, 14) to (start + 0, 15) + = (c1 - c2) +- Code(Counter(1)) at (prev + 0, 19) to (start + 0, 24) +- Code(Counter(2)) at (prev + 1, 9) to (start + 1, 15) +- Code(Expression(0, Sub)) at (prev + 4, 5) to (start + 0, 11) + = (c1 - c2) +- Code(Expression(1, Add)) at (prev + 1, 1) to (start + 0, 2) + = (c2 + (c1 - c2)) + diff --git a/tests/coverage/branch/let-else.coverage b/tests/coverage/branch/let-else.coverage new file mode 100644 index 0000000000000..83730e1dfbafe --- /dev/null +++ b/tests/coverage/branch/let-else.coverage @@ -0,0 +1,37 @@ + LL| |#![feature(coverage_attribute)] + LL| |//@ edition: 2021 + LL| |//@ compile-flags: -Zcoverage-options=branch + LL| |//@ llvm-cov-flags: --show-branches=count + LL| | + LL| |macro_rules! no_merge { + LL| | () => { + LL| | for _ in 0..1 {} + LL| | }; + LL| |} + LL| | + LL| 3|fn let_else(value: Option<&str>) { + LL| 3| no_merge!(); + LL| | + LL| 3| let Some(x) = value else { + ^2 + LL| 1| say("none"); + LL| 1| return; + LL| | }; + LL| | + LL| 2| say(x); + LL| 3|} + LL| | + LL| |#[coverage(off)] + LL| |fn say(message: &str) { + LL| | core::hint::black_box(message); + LL| |} + LL| | + LL| |#[coverage(off)] + LL| |fn main() { + LL| | let_else(Some("x")); + LL| | let_else(Some("x")); + LL| | let_else(None); + LL| |} + LL| | + LL| |// FIXME(#124118) Actually instrument let-else for branch coverage. + diff --git a/tests/coverage/branch/let-else.rs b/tests/coverage/branch/let-else.rs new file mode 100644 index 0000000000000..af0665d8241eb --- /dev/null +++ b/tests/coverage/branch/let-else.rs @@ -0,0 +1,35 @@ +#![feature(coverage_attribute)] +//@ edition: 2021 +//@ compile-flags: -Zcoverage-options=branch +//@ llvm-cov-flags: --show-branches=count + +macro_rules! no_merge { + () => { + for _ in 0..1 {} + }; +} + +fn let_else(value: Option<&str>) { + no_merge!(); + + let Some(x) = value else { + say("none"); + return; + }; + + say(x); +} + +#[coverage(off)] +fn say(message: &str) { + core::hint::black_box(message); +} + +#[coverage(off)] +fn main() { + let_else(Some("x")); + let_else(Some("x")); + let_else(None); +} + +// FIXME(#124118) Actually instrument let-else for branch coverage. From 7f432dfb23f264f5c368464f849663a518750a93 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 17 Apr 2024 11:41:40 +1000 Subject: [PATCH 06/13] coverage: Branch coverage test for if-let and let-chains --- tests/coverage/branch/if-let.cov-map | 41 ++++++++++++++++++ tests/coverage/branch/if-let.coverage | 62 +++++++++++++++++++++++++++ tests/coverage/branch/if-let.rs | 58 +++++++++++++++++++++++++ 3 files changed, 161 insertions(+) create mode 100644 tests/coverage/branch/if-let.cov-map create mode 100644 tests/coverage/branch/if-let.coverage create mode 100644 tests/coverage/branch/if-let.rs diff --git a/tests/coverage/branch/if-let.cov-map b/tests/coverage/branch/if-let.cov-map new file mode 100644 index 0000000000000..c12df8d98018d --- /dev/null +++ b/tests/coverage/branch/if-let.cov-map @@ -0,0 +1,41 @@ +Function name: if_let::if_let +Raw bytes (38): 0x[01, 01, 02, 05, 09, 09, 02, 06, 01, 0c, 01, 01, 10, 02, 03, 11, 00, 12, 05, 00, 16, 00, 1b, 02, 00, 1c, 02, 06, 09, 02, 0c, 02, 06, 07, 03, 05, 01, 02] +Number of files: 1 +- file 0 => global file 1 +Number of expressions: 2 +- expression 0 operands: lhs = Counter(1), rhs = Counter(2) +- expression 1 operands: lhs = Counter(2), rhs = Expression(0, Sub) +Number of file 0 mappings: 6 +- Code(Counter(0)) at (prev + 12, 1) to (start + 1, 16) +- Code(Expression(0, Sub)) at (prev + 3, 17) to (start + 0, 18) + = (c1 - c2) +- Code(Counter(1)) at (prev + 0, 22) to (start + 0, 27) +- Code(Expression(0, Sub)) at (prev + 0, 28) to (start + 2, 6) + = (c1 - c2) +- Code(Counter(2)) at (prev + 2, 12) to (start + 2, 6) +- Code(Expression(1, Add)) at (prev + 3, 5) to (start + 1, 2) + = (c2 + (c1 - c2)) + +Function name: if_let::if_let_chain +Raw bytes (52): 0x[01, 01, 04, 01, 05, 05, 09, 0f, 0d, 05, 09, 08, 01, 17, 01, 00, 33, 02, 01, 11, 00, 12, 01, 00, 16, 00, 17, 0d, 01, 15, 00, 16, 02, 00, 1a, 00, 1b, 0d, 01, 05, 03, 06, 0f, 03, 0c, 02, 06, 0b, 03, 05, 01, 02] +Number of files: 1 +- file 0 => global file 1 +Number of expressions: 4 +- expression 0 operands: lhs = Counter(0), rhs = Counter(1) +- expression 1 operands: lhs = Counter(1), rhs = Counter(2) +- expression 2 operands: lhs = Expression(3, Add), rhs = Counter(3) +- expression 3 operands: lhs = Counter(1), rhs = Counter(2) +Number of file 0 mappings: 8 +- Code(Counter(0)) at (prev + 23, 1) to (start + 0, 51) +- Code(Expression(0, Sub)) at (prev + 1, 17) to (start + 0, 18) + = (c0 - c1) +- Code(Counter(0)) at (prev + 0, 22) to (start + 0, 23) +- Code(Counter(3)) at (prev + 1, 21) to (start + 0, 22) +- Code(Expression(0, Sub)) at (prev + 0, 26) to (start + 0, 27) + = (c0 - c1) +- Code(Counter(3)) at (prev + 1, 5) to (start + 3, 6) +- Code(Expression(3, Add)) at (prev + 3, 12) to (start + 2, 6) + = (c1 + c2) +- Code(Expression(2, Add)) at (prev + 3, 5) to (start + 1, 2) + = ((c1 + c2) + c3) + diff --git a/tests/coverage/branch/if-let.coverage b/tests/coverage/branch/if-let.coverage new file mode 100644 index 0000000000000..f30c5d34eca17 --- /dev/null +++ b/tests/coverage/branch/if-let.coverage @@ -0,0 +1,62 @@ + LL| |#![feature(coverage_attribute, let_chains)] + LL| |//@ edition: 2021 + LL| |//@ compile-flags: -Zcoverage-options=branch + LL| |//@ llvm-cov-flags: --show-branches=count + LL| | + LL| |macro_rules! no_merge { + LL| | () => { + LL| | for _ in 0..1 {} + LL| | }; + LL| |} + LL| | + LL| 3|fn if_let(input: Option<&str>) { + LL| 3| no_merge!(); + LL| | + LL| 3| if let Some(x) = input { + ^2 + LL| 2| say(x); + LL| 2| } else { + LL| 1| say("none"); + LL| 1| } + LL| 3| say("done"); + LL| 3|} + LL| | + LL| 15|fn if_let_chain(a: Option<&str>, b: Option<&str>) { + LL| 15| if let Some(x) = a + ^12 + LL| 12| && let Some(y) = b + ^8 + LL| 8| { + LL| 8| say(x); + LL| 8| say(y); + LL| 8| } else { + LL| 7| say("not both"); + LL| 7| } + LL| 15| say("done"); + LL| 15|} + LL| | + LL| |#[coverage(off)] + LL| |fn say(message: &str) { + LL| | core::hint::black_box(message); + LL| |} + LL| | + LL| |#[coverage(off)] + LL| |fn main() { + LL| | if_let(Some("x")); + LL| | if_let(Some("x")); + LL| | if_let(None); + LL| | + LL| | for _ in 0..8 { + LL| | if_let_chain(Some("a"), Some("b")); + LL| | } + LL| | for _ in 0..4 { + LL| | if_let_chain(Some("a"), None); + LL| | } + LL| | for _ in 0..2 { + LL| | if_let_chain(None, Some("b")); + LL| | } + LL| | if_let_chain(None, None); + LL| |} + LL| | + LL| |// FIXME(#124118) Actually instrument if-let and let-chains for branch coverage. + diff --git a/tests/coverage/branch/if-let.rs b/tests/coverage/branch/if-let.rs new file mode 100644 index 0000000000000..13db00a82b126 --- /dev/null +++ b/tests/coverage/branch/if-let.rs @@ -0,0 +1,58 @@ +#![feature(coverage_attribute, let_chains)] +//@ edition: 2021 +//@ compile-flags: -Zcoverage-options=branch +//@ llvm-cov-flags: --show-branches=count + +macro_rules! no_merge { + () => { + for _ in 0..1 {} + }; +} + +fn if_let(input: Option<&str>) { + no_merge!(); + + if let Some(x) = input { + say(x); + } else { + say("none"); + } + say("done"); +} + +fn if_let_chain(a: Option<&str>, b: Option<&str>) { + if let Some(x) = a + && let Some(y) = b + { + say(x); + say(y); + } else { + say("not both"); + } + say("done"); +} + +#[coverage(off)] +fn say(message: &str) { + core::hint::black_box(message); +} + +#[coverage(off)] +fn main() { + if_let(Some("x")); + if_let(Some("x")); + if_let(None); + + for _ in 0..8 { + if_let_chain(Some("a"), Some("b")); + } + for _ in 0..4 { + if_let_chain(Some("a"), None); + } + for _ in 0..2 { + if_let_chain(None, Some("b")); + } + if_let_chain(None, None); +} + +// FIXME(#124118) Actually instrument if-let and let-chains for branch coverage. From 3de87feba23c6b9df520f284b51390c02ea8d12a Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 17 Apr 2024 12:32:11 +1000 Subject: [PATCH 07/13] coverage: Branch coverage tests for match arms --- tests/coverage/branch/match-arms.cov-map | 92 ++++++++++++++++ tests/coverage/branch/match-arms.coverage | 105 +++++++++++++++++++ tests/coverage/branch/match-arms.rs | 90 ++++++++++++++++ tests/coverage/branch/match-trivial.cov-map | 17 +++ tests/coverage/branch/match-trivial.coverage | 49 +++++++++ tests/coverage/branch/match-trivial.rs | 48 +++++++++ 6 files changed, 401 insertions(+) create mode 100644 tests/coverage/branch/match-arms.cov-map create mode 100644 tests/coverage/branch/match-arms.coverage create mode 100644 tests/coverage/branch/match-arms.rs create mode 100644 tests/coverage/branch/match-trivial.cov-map create mode 100644 tests/coverage/branch/match-trivial.coverage create mode 100644 tests/coverage/branch/match-trivial.rs diff --git a/tests/coverage/branch/match-arms.cov-map b/tests/coverage/branch/match-arms.cov-map new file mode 100644 index 0000000000000..1f17f11baaa07 --- /dev/null +++ b/tests/coverage/branch/match-arms.cov-map @@ -0,0 +1,92 @@ +Function name: match_arms::guards +Raw bytes (88): 0x[01, 01, 08, 07, 15, 0b, 11, 0f, 0d, 00, 09, 17, 25, 1b, 21, 1f, 1d, 03, 19, 0c, 01, 30, 01, 01, 10, 29, 03, 0b, 00, 10, 19, 01, 11, 00, 29, 20, 19, 09, 00, 17, 00, 1b, 1d, 01, 11, 00, 29, 20, 1d, 0d, 00, 17, 00, 1b, 21, 01, 11, 00, 29, 20, 21, 11, 00, 17, 00, 1b, 25, 01, 11, 00, 29, 20, 25, 15, 00, 17, 00, 1b, 03, 01, 0e, 00, 18, 13, 03, 05, 01, 02] +Number of files: 1 +- file 0 => global file 1 +Number of expressions: 8 +- expression 0 operands: lhs = Expression(1, Add), rhs = Counter(5) +- expression 1 operands: lhs = Expression(2, Add), rhs = Counter(4) +- expression 2 operands: lhs = Expression(3, Add), rhs = Counter(3) +- expression 3 operands: lhs = Zero, rhs = Counter(2) +- expression 4 operands: lhs = Expression(5, Add), rhs = Counter(9) +- expression 5 operands: lhs = Expression(6, Add), rhs = Counter(8) +- expression 6 operands: lhs = Expression(7, Add), rhs = Counter(7) +- expression 7 operands: lhs = Expression(0, Add), rhs = Counter(6) +Number of file 0 mappings: 12 +- Code(Counter(0)) at (prev + 48, 1) to (start + 1, 16) +- Code(Counter(10)) at (prev + 3, 11) to (start + 0, 16) +- Code(Counter(6)) at (prev + 1, 17) to (start + 0, 41) +- Branch { true: Counter(6), false: Counter(2) } at (prev + 0, 23) to (start + 0, 27) + true = c6 + false = c2 +- Code(Counter(7)) at (prev + 1, 17) to (start + 0, 41) +- Branch { true: Counter(7), false: Counter(3) } at (prev + 0, 23) to (start + 0, 27) + true = c7 + false = c3 +- Code(Counter(8)) at (prev + 1, 17) to (start + 0, 41) +- Branch { true: Counter(8), false: Counter(4) } at (prev + 0, 23) to (start + 0, 27) + true = c8 + false = c4 +- Code(Counter(9)) at (prev + 1, 17) to (start + 0, 41) +- Branch { true: Counter(9), false: Counter(5) } at (prev + 0, 23) to (start + 0, 27) + true = c9 + false = c5 +- Code(Expression(0, Add)) at (prev + 1, 14) to (start + 0, 24) + = ((((Zero + c2) + c3) + c4) + c5) +- Code(Expression(4, Add)) at (prev + 3, 5) to (start + 1, 2) + = ((((((((Zero + c2) + c3) + c4) + c5) + c6) + c7) + c8) + c9) + +Function name: match_arms::match_arms +Raw bytes (51): 0x[01, 01, 06, 05, 07, 0b, 11, 09, 0d, 13, 02, 17, 09, 11, 0d, 07, 01, 18, 01, 01, 10, 05, 03, 0b, 00, 10, 11, 01, 11, 00, 21, 0d, 01, 11, 00, 21, 09, 01, 11, 00, 21, 02, 01, 11, 00, 21, 0f, 03, 05, 01, 02] +Number of files: 1 +- file 0 => global file 1 +Number of expressions: 6 +- expression 0 operands: lhs = Counter(1), rhs = Expression(1, Add) +- expression 1 operands: lhs = Expression(2, Add), rhs = Counter(4) +- expression 2 operands: lhs = Counter(2), rhs = Counter(3) +- expression 3 operands: lhs = Expression(4, Add), rhs = Expression(0, Sub) +- expression 4 operands: lhs = Expression(5, Add), rhs = Counter(2) +- expression 5 operands: lhs = Counter(4), rhs = Counter(3) +Number of file 0 mappings: 7 +- Code(Counter(0)) at (prev + 24, 1) to (start + 1, 16) +- Code(Counter(1)) at (prev + 3, 11) to (start + 0, 16) +- Code(Counter(4)) at (prev + 1, 17) to (start + 0, 33) +- Code(Counter(3)) at (prev + 1, 17) to (start + 0, 33) +- Code(Counter(2)) at (prev + 1, 17) to (start + 0, 33) +- Code(Expression(0, Sub)) at (prev + 1, 17) to (start + 0, 33) + = (c1 - ((c2 + c3) + c4)) +- Code(Expression(3, Add)) at (prev + 3, 5) to (start + 1, 2) + = (((c4 + c3) + c2) + (c1 - ((c2 + c3) + c4))) + +Function name: match_arms::or_patterns +Raw bytes (75): 0x[01, 01, 0d, 11, 0d, 05, 2f, 33, 11, 09, 0d, 09, 2a, 05, 2f, 33, 11, 09, 0d, 03, 27, 09, 2a, 05, 2f, 33, 11, 09, 0d, 09, 01, 25, 01, 01, 10, 05, 03, 0b, 00, 10, 11, 01, 11, 00, 12, 0d, 00, 1e, 00, 1f, 03, 00, 24, 00, 2e, 09, 01, 11, 00, 12, 2a, 00, 1e, 00, 1f, 27, 00, 24, 00, 2e, 23, 03, 05, 01, 02] +Number of files: 1 +- file 0 => global file 1 +Number of expressions: 13 +- expression 0 operands: lhs = Counter(4), rhs = Counter(3) +- expression 1 operands: lhs = Counter(1), rhs = Expression(11, Add) +- expression 2 operands: lhs = Expression(12, Add), rhs = Counter(4) +- expression 3 operands: lhs = Counter(2), rhs = Counter(3) +- expression 4 operands: lhs = Counter(2), rhs = Expression(10, Sub) +- expression 5 operands: lhs = Counter(1), rhs = Expression(11, Add) +- expression 6 operands: lhs = Expression(12, Add), rhs = Counter(4) +- expression 7 operands: lhs = Counter(2), rhs = Counter(3) +- expression 8 operands: lhs = Expression(0, Add), rhs = Expression(9, Add) +- expression 9 operands: lhs = Counter(2), rhs = Expression(10, Sub) +- expression 10 operands: lhs = Counter(1), rhs = Expression(11, Add) +- expression 11 operands: lhs = Expression(12, Add), rhs = Counter(4) +- expression 12 operands: lhs = Counter(2), rhs = Counter(3) +Number of file 0 mappings: 9 +- Code(Counter(0)) at (prev + 37, 1) to (start + 1, 16) +- Code(Counter(1)) at (prev + 3, 11) to (start + 0, 16) +- Code(Counter(4)) at (prev + 1, 17) to (start + 0, 18) +- Code(Counter(3)) at (prev + 0, 30) to (start + 0, 31) +- Code(Expression(0, Add)) at (prev + 0, 36) to (start + 0, 46) + = (c4 + c3) +- Code(Counter(2)) at (prev + 1, 17) to (start + 0, 18) +- Code(Expression(10, Sub)) at (prev + 0, 30) to (start + 0, 31) + = (c1 - ((c2 + c3) + c4)) +- Code(Expression(9, Add)) at (prev + 0, 36) to (start + 0, 46) + = (c2 + (c1 - ((c2 + c3) + c4))) +- Code(Expression(8, Add)) at (prev + 3, 5) to (start + 1, 2) + = ((c4 + c3) + (c2 + (c1 - ((c2 + c3) + c4)))) + diff --git a/tests/coverage/branch/match-arms.coverage b/tests/coverage/branch/match-arms.coverage new file mode 100644 index 0000000000000..ea8a6f97ab154 --- /dev/null +++ b/tests/coverage/branch/match-arms.coverage @@ -0,0 +1,105 @@ + LL| |#![feature(coverage_attribute)] + LL| |//@ edition: 2021 + LL| |//@ compile-flags: -Zcoverage-options=branch + LL| |//@ llvm-cov-flags: --show-branches=count + LL| | + LL| |// Tests for branch coverage of various kinds of match arms. + LL| | + LL| |// Helper macro to prevent start-of-function spans from being merged into + LL| |// spans on the lines we care about. + LL| |macro_rules! no_merge { + LL| | () => { + LL| | for _ in 0..1 {} + LL| | }; + LL| |} + LL| | + LL| |#[derive(Clone, Copy, Debug)] + LL| |enum Enum { + LL| | A(u32), + LL| | B(u32), + LL| | C(u32), + LL| | D(u32), + LL| |} + LL| | + LL| 15|fn match_arms(value: Enum) { + LL| 15| no_merge!(); + LL| | + LL| 15| match value { + LL| 8| Enum::D(d) => consume(d), + LL| 4| Enum::C(c) => consume(c), + LL| 2| Enum::B(b) => consume(b), + LL| 1| Enum::A(a) => consume(a), + LL| | } + LL| | + LL| 15| consume(0); + LL| 15|} + LL| | + LL| 15|fn or_patterns(value: Enum) { + LL| 15| no_merge!(); + LL| | + LL| 15| match value { + LL| 12| Enum::D(x) | Enum::C(x) => consume(x), + ^8 ^4 + LL| 3| Enum::B(y) | Enum::A(y) => consume(y), + ^2 ^1 + LL| | } + LL| | + LL| 15| consume(0); + LL| 15|} + LL| | + LL| 45|fn guards(value: Enum, cond: bool) { + LL| 45| no_merge!(); + LL| | + LL| 3| match value { + LL| 8| Enum::D(d) if cond => consume(d), + ------------------ + | Branch (LL:23): [True: 8, False: 16] + ------------------ + LL| 4| Enum::C(c) if cond => consume(c), + ------------------ + | Branch (LL:23): [True: 4, False: 8] + ------------------ + LL| 2| Enum::B(b) if cond => consume(b), + ------------------ + | Branch (LL:23): [True: 2, False: 4] + ------------------ + LL| 1| Enum::A(a) if cond => consume(a), + ------------------ + | Branch (LL:23): [True: 1, False: 2] + ------------------ + LL| 30| _ => consume(0), + LL| | } + LL| | + LL| 45| consume(0); + LL| 45|} + LL| | + LL| |#[coverage(off)] + LL| |fn consume(x: T) { + LL| | core::hint::black_box(x); + LL| |} + LL| | + LL| |#[coverage(off)] + LL| |fn main() { + LL| | #[coverage(off)] + LL| | fn call_everything(e: Enum) { + LL| | match_arms(e); + LL| | or_patterns(e); + LL| | for cond in [false, false, true] { + LL| | guards(e, cond); + LL| | } + LL| | } + LL| | + LL| | call_everything(Enum::A(0)); + LL| | for b in 0..2 { + LL| | call_everything(Enum::B(b)); + LL| | } + LL| | for c in 0..4 { + LL| | call_everything(Enum::C(c)); + LL| | } + LL| | for d in 0..8 { + LL| | call_everything(Enum::D(d)); + LL| | } + LL| |} + LL| | + LL| |// FIXME(#124118) Actually instrument match arms for branch coverage. + diff --git a/tests/coverage/branch/match-arms.rs b/tests/coverage/branch/match-arms.rs new file mode 100644 index 0000000000000..63151f59ffe9b --- /dev/null +++ b/tests/coverage/branch/match-arms.rs @@ -0,0 +1,90 @@ +#![feature(coverage_attribute)] +//@ edition: 2021 +//@ compile-flags: -Zcoverage-options=branch +//@ llvm-cov-flags: --show-branches=count + +// Tests for branch coverage of various kinds of match arms. + +// Helper macro to prevent start-of-function spans from being merged into +// spans on the lines we care about. +macro_rules! no_merge { + () => { + for _ in 0..1 {} + }; +} + +#[derive(Clone, Copy, Debug)] +enum Enum { + A(u32), + B(u32), + C(u32), + D(u32), +} + +fn match_arms(value: Enum) { + no_merge!(); + + match value { + Enum::D(d) => consume(d), + Enum::C(c) => consume(c), + Enum::B(b) => consume(b), + Enum::A(a) => consume(a), + } + + consume(0); +} + +fn or_patterns(value: Enum) { + no_merge!(); + + match value { + Enum::D(x) | Enum::C(x) => consume(x), + Enum::B(y) | Enum::A(y) => consume(y), + } + + consume(0); +} + +fn guards(value: Enum, cond: bool) { + no_merge!(); + + match value { + Enum::D(d) if cond => consume(d), + Enum::C(c) if cond => consume(c), + Enum::B(b) if cond => consume(b), + Enum::A(a) if cond => consume(a), + _ => consume(0), + } + + consume(0); +} + +#[coverage(off)] +fn consume(x: T) { + core::hint::black_box(x); +} + +#[coverage(off)] +fn main() { + #[coverage(off)] + fn call_everything(e: Enum) { + match_arms(e); + or_patterns(e); + for cond in [false, false, true] { + guards(e, cond); + } + } + + call_everything(Enum::A(0)); + for b in 0..2 { + call_everything(Enum::B(b)); + } + for c in 0..4 { + call_everything(Enum::C(c)); + } + for d in 0..8 { + call_everything(Enum::D(d)); + } +} + +// FIXME(#124118) Actually instrument match arms for branch coverage. diff --git a/tests/coverage/branch/match-trivial.cov-map b/tests/coverage/branch/match-trivial.cov-map new file mode 100644 index 0000000000000..1136a529b255c --- /dev/null +++ b/tests/coverage/branch/match-trivial.cov-map @@ -0,0 +1,17 @@ +Function name: match_trivial::_uninhabited (unused) +Raw bytes (9): 0x[01, 01, 00, 01, 00, 16, 01, 01, 10] +Number of files: 1 +- file 0 => global file 1 +Number of expressions: 0 +Number of file 0 mappings: 1 +- Code(Zero) at (prev + 22, 1) to (start + 1, 16) + +Function name: match_trivial::trivial +Raw bytes (14): 0x[01, 01, 00, 02, 01, 1e, 01, 01, 10, 05, 03, 0b, 05, 02] +Number of files: 1 +- file 0 => global file 1 +Number of expressions: 0 +Number of file 0 mappings: 2 +- Code(Counter(0)) at (prev + 30, 1) to (start + 1, 16) +- Code(Counter(1)) at (prev + 3, 11) to (start + 5, 2) + diff --git a/tests/coverage/branch/match-trivial.coverage b/tests/coverage/branch/match-trivial.coverage new file mode 100644 index 0000000000000..4ffb172e1b675 --- /dev/null +++ b/tests/coverage/branch/match-trivial.coverage @@ -0,0 +1,49 @@ + LL| |#![feature(coverage_attribute)] + LL| |//@ edition: 2021 + LL| |//@ compile-flags: -Zcoverage-options=branch + LL| |//@ llvm-cov-flags: --show-branches=count + LL| | + LL| |// When instrumenting match expressions for branch coverage, make sure we don't + LL| |// cause an ICE or produce weird coverage output for matches with <2 arms. + LL| | + LL| |// Helper macro to prevent start-of-function spans from being merged into + LL| |// spans on the lines we care about. + LL| |macro_rules! no_merge { + LL| | () => { + LL| | for _ in 0..1 {} + LL| | }; + LL| |} + LL| | + LL| |enum Uninhabited {} + LL| |enum Trivial { + LL| | Value, + LL| |} + LL| | + LL| 0|fn _uninhabited(x: Uninhabited) { + LL| 0| no_merge!(); + LL| | + LL| | match x {} + LL| | + LL| | consume("done"); + LL| |} + LL| | + LL| 1|fn trivial(x: Trivial) { + LL| 1| no_merge!(); + LL| | + LL| 1| match x { + LL| 1| Trivial::Value => consume("trivial"), + LL| 1| } + LL| 1| + LL| 1| consume("done"); + LL| 1|} + LL| | + LL| |#[coverage(off)] + LL| |fn consume(x: T) { + LL| | core::hint::black_box(x); + LL| |} + LL| | + LL| |#[coverage(off)] + LL| |fn main() { + LL| | trivial(Trivial::Value); + LL| |} + diff --git a/tests/coverage/branch/match-trivial.rs b/tests/coverage/branch/match-trivial.rs new file mode 100644 index 0000000000000..db8887a26b7a4 --- /dev/null +++ b/tests/coverage/branch/match-trivial.rs @@ -0,0 +1,48 @@ +#![feature(coverage_attribute)] +//@ edition: 2021 +//@ compile-flags: -Zcoverage-options=branch +//@ llvm-cov-flags: --show-branches=count + +// When instrumenting match expressions for branch coverage, make sure we don't +// cause an ICE or produce weird coverage output for matches with <2 arms. + +// Helper macro to prevent start-of-function spans from being merged into +// spans on the lines we care about. +macro_rules! no_merge { + () => { + for _ in 0..1 {} + }; +} + +enum Uninhabited {} +enum Trivial { + Value, +} + +fn _uninhabited(x: Uninhabited) { + no_merge!(); + + match x {} + + consume("done"); +} + +fn trivial(x: Trivial) { + no_merge!(); + + match x { + Trivial::Value => consume("trivial"), + } + + consume("done"); +} + +#[coverage(off)] +fn consume(x: T) { + core::hint::black_box(x); +} + +#[coverage(off)] +fn main() { + trivial(Trivial::Value); +} From da37b14121d0fed6e49a4ed27abd2b9e7fc1c486 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 19 Apr 2024 14:50:25 +1000 Subject: [PATCH 08/13] coverage: Move mir-opt coverage tests into a subdirectory --- .../instrument_coverage.bar.InstrumentCoverage.diff | 0 .../instrument_coverage.main.InstrumentCoverage.diff | 0 tests/mir-opt/{ => coverage}/instrument_coverage.rs | 0 .../instrument_coverage_cleanup.main.CleanupPostBorrowck.diff | 0 .../instrument_coverage_cleanup.main.InstrumentCoverage.diff | 0 tests/mir-opt/{ => coverage}/instrument_coverage_cleanup.rs | 0 6 files changed, 0 insertions(+), 0 deletions(-) rename tests/mir-opt/{ => coverage}/instrument_coverage.bar.InstrumentCoverage.diff (100%) rename tests/mir-opt/{ => coverage}/instrument_coverage.main.InstrumentCoverage.diff (100%) rename tests/mir-opt/{ => coverage}/instrument_coverage.rs (100%) rename tests/mir-opt/{ => coverage}/instrument_coverage_cleanup.main.CleanupPostBorrowck.diff (100%) rename tests/mir-opt/{ => coverage}/instrument_coverage_cleanup.main.InstrumentCoverage.diff (100%) rename tests/mir-opt/{ => coverage}/instrument_coverage_cleanup.rs (100%) diff --git a/tests/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff b/tests/mir-opt/coverage/instrument_coverage.bar.InstrumentCoverage.diff similarity index 100% rename from tests/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff rename to tests/mir-opt/coverage/instrument_coverage.bar.InstrumentCoverage.diff diff --git a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff b/tests/mir-opt/coverage/instrument_coverage.main.InstrumentCoverage.diff similarity index 100% rename from tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff rename to tests/mir-opt/coverage/instrument_coverage.main.InstrumentCoverage.diff diff --git a/tests/mir-opt/instrument_coverage.rs b/tests/mir-opt/coverage/instrument_coverage.rs similarity index 100% rename from tests/mir-opt/instrument_coverage.rs rename to tests/mir-opt/coverage/instrument_coverage.rs diff --git a/tests/mir-opt/instrument_coverage_cleanup.main.CleanupPostBorrowck.diff b/tests/mir-opt/coverage/instrument_coverage_cleanup.main.CleanupPostBorrowck.diff similarity index 100% rename from tests/mir-opt/instrument_coverage_cleanup.main.CleanupPostBorrowck.diff rename to tests/mir-opt/coverage/instrument_coverage_cleanup.main.CleanupPostBorrowck.diff diff --git a/tests/mir-opt/instrument_coverage_cleanup.main.InstrumentCoverage.diff b/tests/mir-opt/coverage/instrument_coverage_cleanup.main.InstrumentCoverage.diff similarity index 100% rename from tests/mir-opt/instrument_coverage_cleanup.main.InstrumentCoverage.diff rename to tests/mir-opt/coverage/instrument_coverage_cleanup.main.InstrumentCoverage.diff diff --git a/tests/mir-opt/instrument_coverage_cleanup.rs b/tests/mir-opt/coverage/instrument_coverage_cleanup.rs similarity index 100% rename from tests/mir-opt/instrument_coverage_cleanup.rs rename to tests/mir-opt/coverage/instrument_coverage_cleanup.rs From a892c2387e64ba080a6503d422931f3a0916826a Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 19 Apr 2024 15:35:23 +1000 Subject: [PATCH 09/13] coverage: Add a mir-opt test for branch coverage of match arms --- ...ch_match_arms.main.InstrumentCoverage.diff | 138 ++++++++++++++++++ tests/mir-opt/coverage/branch_match_arms.rs | 27 ++++ 2 files changed, 165 insertions(+) create mode 100644 tests/mir-opt/coverage/branch_match_arms.main.InstrumentCoverage.diff create mode 100644 tests/mir-opt/coverage/branch_match_arms.rs diff --git a/tests/mir-opt/coverage/branch_match_arms.main.InstrumentCoverage.diff b/tests/mir-opt/coverage/branch_match_arms.main.InstrumentCoverage.diff new file mode 100644 index 0000000000000..e60f71f47b1ed --- /dev/null +++ b/tests/mir-opt/coverage/branch_match_arms.main.InstrumentCoverage.diff @@ -0,0 +1,138 @@ +- // MIR for `main` before InstrumentCoverage ++ // MIR for `main` after InstrumentCoverage + + fn main() -> () { + let mut _0: (); + let mut _1: Enum; + let mut _2: isize; + let _3: u32; + let mut _4: u32; + let _5: u32; + let mut _6: u32; + let _7: u32; + let mut _8: u32; + let _9: u32; + let mut _10: u32; + scope 1 { + debug d => _3; + } + scope 2 { + debug c => _5; + } + scope 3 { + debug b => _7; + } + scope 4 { + debug a => _9; + } + ++ coverage ExpressionId(0) => Expression { lhs: Counter(1), op: Add, rhs: Counter(2) }; ++ coverage ExpressionId(1) => Expression { lhs: Expression(0), op: Add, rhs: Counter(3) }; ++ coverage ExpressionId(2) => Expression { lhs: Counter(0), op: Subtract, rhs: Expression(1) }; ++ coverage ExpressionId(3) => Expression { lhs: Counter(3), op: Add, rhs: Counter(2) }; ++ coverage ExpressionId(4) => Expression { lhs: Expression(3), op: Add, rhs: Counter(1) }; ++ coverage ExpressionId(5) => Expression { lhs: Expression(4), op: Add, rhs: Expression(2) }; ++ coverage Code(Counter(0)) => $DIR/branch_match_arms.rs:14:1 - 15:21; ++ coverage Code(Counter(3)) => $DIR/branch_match_arms.rs:16:17 - 16:33; ++ coverage Code(Counter(2)) => $DIR/branch_match_arms.rs:17:17 - 17:33; ++ coverage Code(Counter(1)) => $DIR/branch_match_arms.rs:18:17 - 18:33; ++ coverage Code(Expression(2)) => $DIR/branch_match_arms.rs:19:17 - 19:33; ++ coverage Code(Expression(5)) => $DIR/branch_match_arms.rs:21:1 - 21:2; ++ + bb0: { ++ Coverage::CounterIncrement(0); + StorageLive(_1); + _1 = Enum::A(const 0_u32); + PlaceMention(_1); + _2 = discriminant(_1); + switchInt(move _2) -> [0: bb5, 1: bb4, 2: bb3, 3: bb2, otherwise: bb1]; + } + + bb1: { + FakeRead(ForMatchedPlace(None), _1); + unreachable; + } + + bb2: { ++ Coverage::CounterIncrement(3); + falseEdge -> [real: bb6, imaginary: bb3]; + } + + bb3: { ++ Coverage::CounterIncrement(2); + falseEdge -> [real: bb8, imaginary: bb4]; + } + + bb4: { ++ Coverage::CounterIncrement(1); + falseEdge -> [real: bb10, imaginary: bb5]; + } + + bb5: { ++ Coverage::ExpressionUsed(2); + StorageLive(_9); + _9 = ((_1 as A).0: u32); + StorageLive(_10); + _10 = _9; + _0 = consume(move _10) -> [return: bb12, unwind: bb14]; + } + + bb6: { + StorageLive(_3); + _3 = ((_1 as D).0: u32); + StorageLive(_4); + _4 = _3; + _0 = consume(move _4) -> [return: bb7, unwind: bb14]; + } + + bb7: { + StorageDead(_4); + StorageDead(_3); + goto -> bb13; + } + + bb8: { + StorageLive(_5); + _5 = ((_1 as C).0: u32); + StorageLive(_6); + _6 = _5; + _0 = consume(move _6) -> [return: bb9, unwind: bb14]; + } + + bb9: { + StorageDead(_6); + StorageDead(_5); + goto -> bb13; + } + + bb10: { + StorageLive(_7); + _7 = ((_1 as B).0: u32); + StorageLive(_8); + _8 = _7; + _0 = consume(move _8) -> [return: bb11, unwind: bb14]; + } + + bb11: { + StorageDead(_8); + StorageDead(_7); + goto -> bb13; + } + + bb12: { + StorageDead(_10); + StorageDead(_9); + goto -> bb13; + } + + bb13: { ++ Coverage::ExpressionUsed(5); + StorageDead(_1); + return; + } + + bb14 (cleanup): { + resume; + } + } + diff --git a/tests/mir-opt/coverage/branch_match_arms.rs b/tests/mir-opt/coverage/branch_match_arms.rs new file mode 100644 index 0000000000000..18764b38d6e3e --- /dev/null +++ b/tests/mir-opt/coverage/branch_match_arms.rs @@ -0,0 +1,27 @@ +#![feature(coverage_attribute)] +//@ test-mir-pass: InstrumentCoverage +//@ compile-flags: -Cinstrument-coverage -Zno-profiler-runtime -Zcoverage-options=branch +// skip-filecheck + +enum Enum { + A(u32), + B(u32), + C(u32), + D(u32), +} + +// EMIT_MIR branch_match_arms.main.InstrumentCoverage.diff +fn main() { + match Enum::A(0) { + Enum::D(d) => consume(d), + Enum::C(c) => consume(c), + Enum::B(b) => consume(b), + Enum::A(a) => consume(a), + } +} + +#[inline(never)] +#[coverage(off)] +fn consume(x: u32) { + core::hint::black_box(x); +} From 97bf5536827ea7a1ba6b7cf856dd2b22184d2527 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sun, 21 Apr 2024 12:06:03 +1000 Subject: [PATCH 10/13] coverage: Detach MC/DC branch spans from regular branch spans MC/DC's reliance on the existing branch coverage types is making it much harder to improve branch coverage. --- compiler/rustc_middle/src/mir/coverage.rs | 4 +- compiler/rustc_middle/src/mir/pretty.rs | 2 +- .../rustc_mir_build/src/build/coverageinfo.rs | 47 +++++------- .../rustc_mir_transform/src/coverage/mod.rs | 16 +++- .../rustc_mir_transform/src/coverage/spans.rs | 10 ++- .../src/coverage/spans/from_mir.rs | 74 ++++++++++++++----- 6 files changed, 101 insertions(+), 52 deletions(-) diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index b1d0c815ae06b..04011fd41948e 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -314,7 +314,9 @@ impl Default for ConditionInfo { #[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)] pub struct MCDCBranchSpan { pub span: Span, - pub condition_info: ConditionInfo, + /// If `None`, this actually represents a normal branch span inserted for + /// code that was too complex for MC/DC. + pub condition_info: Option, pub true_marker: BlockMarkerId, pub false_marker: BlockMarkerId, } diff --git a/compiler/rustc_middle/src/mir/pretty.rs b/compiler/rustc_middle/src/mir/pretty.rs index 8291404ebf31f..0c9ceae63c1cd 100644 --- a/compiler/rustc_middle/src/mir/pretty.rs +++ b/compiler/rustc_middle/src/mir/pretty.rs @@ -491,7 +491,7 @@ fn write_coverage_branch_info( writeln!( w, "{INDENT}coverage mcdc branch {{ condition_id: {:?}, true: {true_marker:?}, false: {false_marker:?} }} => {span:?}", - condition_info.condition_id + condition_info.map(|info| info.condition_id) )?; } diff --git a/compiler/rustc_mir_build/src/build/coverageinfo.rs b/compiler/rustc_mir_build/src/build/coverageinfo.rs index 57f809ef7cf8a..15963c1234f8e 100644 --- a/compiler/rustc_mir_build/src/build/coverageinfo.rs +++ b/compiler/rustc_mir_build/src/build/coverageinfo.rs @@ -22,6 +22,7 @@ pub(crate) struct BranchInfoBuilder { num_block_markers: usize, branch_spans: Vec, + mcdc_branch_spans: Vec, mcdc_decision_spans: Vec, mcdc_state: Option, @@ -95,13 +96,7 @@ impl BranchInfoBuilder { } } - fn record_conditions_operation(&mut self, logical_op: LogicalOp, span: Span) { - if let Some(mcdc_state) = self.mcdc_state.as_mut() { - mcdc_state.record_conditions(logical_op, span); - } - } - - fn fetch_condition_info( + fn fetch_mcdc_condition_info( &mut self, tcx: TyCtxt<'_>, true_marker: BlockMarkerId, @@ -121,14 +116,9 @@ impl BranchInfoBuilder { _ => { // Do not generate mcdc mappings and statements for decisions with too many conditions. let rebase_idx = self.mcdc_branch_spans.len() - decision.conditions_num + 1; - let to_normal_branches = self.mcdc_branch_spans.split_off(rebase_idx); - self.branch_spans.extend(to_normal_branches.into_iter().map( - |MCDCBranchSpan { span, true_marker, false_marker, .. }| BranchSpan { - span, - true_marker, - false_marker, - }, - )); + for branch in &mut self.mcdc_branch_spans[rebase_idx..] { + branch.condition_info = None; + } // ConditionInfo of this branch shall also be reset. condition_info = None; @@ -157,7 +147,7 @@ impl BranchInfoBuilder { branch_spans, mcdc_branch_spans, mcdc_decision_spans, - .. + mcdc_state: _, } = self; if num_block_markers == 0 { @@ -355,27 +345,30 @@ impl Builder<'_, '_> { let true_marker = inject_branch_marker(then_block); let false_marker = inject_branch_marker(else_block); - if let Some(condition_info) = - branch_info.fetch_condition_info(self.tcx, true_marker, false_marker) - { + if branch_info.mcdc_state.is_some() { + let condition_info = + branch_info.fetch_mcdc_condition_info(self.tcx, true_marker, false_marker); branch_info.mcdc_branch_spans.push(MCDCBranchSpan { span: source_info.span, condition_info, true_marker, false_marker, }); - } else { - branch_info.branch_spans.push(BranchSpan { - span: source_info.span, - true_marker, - false_marker, - }); + return; } + + branch_info.branch_spans.push(BranchSpan { + span: source_info.span, + true_marker, + false_marker, + }); } pub(crate) fn visit_coverage_branch_operation(&mut self, logical_op: LogicalOp, span: Span) { - if let Some(branch_info) = self.coverage_branch_info.as_mut() { - branch_info.record_conditions_operation(logical_op, span); + if let Some(branch_info) = self.coverage_branch_info.as_mut() + && let Some(mcdc_state) = branch_info.mcdc_state.as_mut() + { + mcdc_state.record_conditions(logical_op, span); } } } diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index 9e8648b0f930d..d1516605fb603 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -149,13 +149,21 @@ fn create_mappings<'tcx>( true_term: term_for_bcb(true_bcb), false_term: term_for_bcb(false_bcb), }, - BcbMappingKind::MCDCBranch { true_bcb, false_bcb, condition_info } => { - MappingKind::MCDCBranch { + BcbMappingKind::MCDCBranch { true_bcb, false_bcb, condition_info: None } => { + MappingKind::Branch { true_term: term_for_bcb(true_bcb), false_term: term_for_bcb(false_bcb), - mcdc_params: condition_info, } } + BcbMappingKind::MCDCBranch { + true_bcb, + false_bcb, + condition_info: Some(mcdc_params), + } => MappingKind::MCDCBranch { + true_term: term_for_bcb(true_bcb), + false_term: term_for_bcb(false_bcb), + mcdc_params, + }, BcbMappingKind::MCDCDecision { bitmap_idx, conditions_num, .. } => { MappingKind::MCDCDecision(DecisionInfo { bitmap_idx, conditions_num }) } @@ -249,7 +257,7 @@ fn inject_mcdc_statements<'tcx>( for (true_bcb, false_bcb, condition_id) in coverage_spans.all_bcb_mappings().filter_map(|mapping| match mapping.kind { BcbMappingKind::MCDCBranch { true_bcb, false_bcb, condition_info } => { - Some((true_bcb, false_bcb, condition_info.condition_id)) + Some((true_bcb, false_bcb, condition_info?.condition_id)) } _ => None, }) diff --git a/compiler/rustc_mir_transform/src/coverage/spans.rs b/compiler/rustc_mir_transform/src/coverage/spans.rs index a4cd8a38c66a2..8d2241783f647 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans.rs @@ -21,7 +21,9 @@ pub(super) enum BcbMappingKind { MCDCBranch { true_bcb: BasicCoverageBlock, false_bcb: BasicCoverageBlock, - condition_info: ConditionInfo, + /// If `None`, this actually represents a normal branch mapping inserted + /// for code that was too complex for MC/DC. + condition_info: Option, }, /// Associates a mcdc decision with its join BCB. MCDCDecision { end_bcbs: BTreeSet, bitmap_idx: u32, conditions_num: u16 }, @@ -85,6 +87,12 @@ pub(super) fn generate_coverage_spans( })); mappings.extend(from_mir::extract_branch_mappings( + mir_body, + hir_info, + basic_coverage_blocks, + )); + + mappings.extend(from_mir::extract_mcdc_mappings( mir_body, hir_info.body_span, basic_coverage_blocks, diff --git a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs index b9919a2ae884e..3c64591d43e9c 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs @@ -366,15 +366,10 @@ impl SpanFromMir { } } -pub(super) fn extract_branch_mappings( +fn resolve_block_markers( + branch_info: &mir::coverage::BranchInfo, mir_body: &mir::Body<'_>, - body_span: Span, - basic_coverage_blocks: &CoverageGraph, -) -> Vec { - let Some(branch_info) = mir_body.coverage_branch_info.as_deref() else { - return vec![]; - }; - +) -> IndexVec> { let mut block_markers = IndexVec::>::from_elem_n( None, branch_info.num_block_markers, @@ -389,6 +384,58 @@ pub(super) fn extract_branch_mappings( } } + block_markers +} + +// FIXME: There is currently a lot of redundancy between +// `extract_branch_mappings` and `extract_mcdc_mappings`. This is needed so +// that they can each be modified without interfering with the other, but in +// the long term we should try to bring them together again when branch coverage +// and MC/DC coverage support are more mature. + +pub(super) fn extract_branch_mappings( + mir_body: &mir::Body<'_>, + hir_info: &ExtractedHirInfo, + basic_coverage_blocks: &CoverageGraph, +) -> Vec { + let Some(branch_info) = mir_body.coverage_branch_info.as_deref() else { return vec![] }; + + let block_markers = resolve_block_markers(branch_info, mir_body); + + branch_info + .branch_spans + .iter() + .filter_map(|&BranchSpan { span: raw_span, true_marker, false_marker }| { + // For now, ignore any branch span that was introduced by + // expansion. This makes things like assert macros less noisy. + if !raw_span.ctxt().outer_expn_data().is_root() { + return None; + } + let (span, _) = + unexpand_into_body_span_with_visible_macro(raw_span, hir_info.body_span)?; + + let bcb_from_marker = + |marker: BlockMarkerId| basic_coverage_blocks.bcb_from_bb(block_markers[marker]?); + + let true_bcb = bcb_from_marker(true_marker)?; + let false_bcb = bcb_from_marker(false_marker)?; + + Some(BcbMapping { kind: BcbMappingKind::Branch { true_bcb, false_bcb }, span }) + }) + .collect::>() +} + +pub(super) fn extract_mcdc_mappings( + mir_body: &mir::Body<'_>, + body_span: Span, + basic_coverage_blocks: &CoverageGraph, +) -> Vec { + let Some(branch_info) = mir_body.coverage_branch_info.as_deref() else { + return vec![]; + }; + + let block_markers = resolve_block_markers(branch_info, mir_body); + let bcb_from_marker = |marker: BlockMarkerId| basic_coverage_blocks.bcb_from_bb(block_markers[marker]?); @@ -406,12 +453,6 @@ pub(super) fn extract_branch_mappings( Some((span, true_bcb, false_bcb)) }; - let branch_filter_map = |&BranchSpan { span: raw_span, true_marker, false_marker }| { - check_branch_bcb(raw_span, true_marker, false_marker).map(|(span, true_bcb, false_bcb)| { - BcbMapping { kind: BcbMappingKind::Branch { true_bcb, false_bcb }, span } - }) - }; - let mcdc_branch_filter_map = |&MCDCBranchSpan { span: raw_span, true_marker, false_marker, condition_info }| { check_branch_bcb(raw_span, true_marker, false_marker).map( @@ -446,10 +487,7 @@ pub(super) fn extract_branch_mappings( }) }; - branch_info - .branch_spans - .iter() - .filter_map(branch_filter_map) + std::iter::empty() .chain(branch_info.mcdc_branch_spans.iter().filter_map(mcdc_branch_filter_map)) .chain(branch_info.mcdc_decision_spans.iter().filter_map(decision_filter_map)) .collect::>() From b5a22be6a3ce55e4bbac62767cda62c38d47e162 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sun, 21 Apr 2024 12:20:09 +1000 Subject: [PATCH 11/13] coverage: Move some helper code into `BranchInfoBuilder` --- .../rustc_mir_build/src/build/coverageinfo.rs | 66 +++++++++++-------- 1 file changed, 40 insertions(+), 26 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/coverageinfo.rs b/compiler/rustc_mir_build/src/build/coverageinfo.rs index 15963c1234f8e..9e9ccd3dc2d01 100644 --- a/compiler/rustc_mir_build/src/build/coverageinfo.rs +++ b/compiler/rustc_mir_build/src/build/coverageinfo.rs @@ -7,13 +7,13 @@ use rustc_middle::mir::coverage::{ BlockMarkerId, BranchSpan, ConditionId, ConditionInfo, CoverageKind, MCDCBranchSpan, MCDCDecisionSpan, }; -use rustc_middle::mir::{self, BasicBlock, UnOp}; +use rustc_middle::mir::{self, BasicBlock, SourceInfo, UnOp}; use rustc_middle::thir::{ExprId, ExprKind, LogicalOp, Thir}; use rustc_middle::ty::TyCtxt; use rustc_span::def_id::LocalDefId; use rustc_span::Span; -use crate::build::Builder; +use crate::build::{Builder, CFG}; use crate::errors::MCDCExceedsConditionNumLimit; pub(crate) struct BranchInfoBuilder { @@ -134,12 +134,42 @@ impl BranchInfoBuilder { condition_info } + fn add_two_way_branch<'tcx>( + &mut self, + cfg: &mut CFG<'tcx>, + source_info: SourceInfo, + true_block: BasicBlock, + false_block: BasicBlock, + ) { + let true_marker = self.inject_block_marker(cfg, source_info, true_block); + let false_marker = self.inject_block_marker(cfg, source_info, false_block); + + self.branch_spans.push(BranchSpan { span: source_info.span, true_marker, false_marker }); + } + fn next_block_marker_id(&mut self) -> BlockMarkerId { let id = BlockMarkerId::from_usize(self.num_block_markers); self.num_block_markers += 1; id } + fn inject_block_marker( + &mut self, + cfg: &mut CFG<'_>, + source_info: SourceInfo, + block: BasicBlock, + ) -> BlockMarkerId { + let id = self.next_block_marker_id(); + + let marker_statement = mir::Statement { + source_info, + kind: mir::StatementKind::Coverage(CoverageKind::BlockMarker { id }), + }; + cfg.push(block, marker_statement); + + id + } + pub(crate) fn into_done(self) -> Option> { let Self { nots: _, @@ -315,7 +345,7 @@ impl Builder<'_, '_> { mut else_block: BasicBlock, ) { // Bail out if branch coverage is not enabled for this function. - let Some(branch_info) = self.coverage_branch_info.as_ref() else { return }; + let Some(branch_info) = self.coverage_branch_info.as_mut() else { return }; // If this condition expression is nested within one or more `!` expressions, // replace it with the enclosing `!` collected by `visit_unary_not`. @@ -325,27 +355,15 @@ impl Builder<'_, '_> { std::mem::swap(&mut then_block, &mut else_block); } } - let source_info = self.source_info(self.thir[expr_id].span); - - // Now that we have `source_info`, we can upgrade to a &mut reference. - let branch_info = self.coverage_branch_info.as_mut().expect("upgrading & to &mut"); - - let mut inject_branch_marker = |block: BasicBlock| { - let id = branch_info.next_block_marker_id(); - - let marker_statement = mir::Statement { - source_info, - kind: mir::StatementKind::Coverage(CoverageKind::BlockMarker { id }), - }; - self.cfg.push(block, marker_statement); - - id - }; - let true_marker = inject_branch_marker(then_block); - let false_marker = inject_branch_marker(else_block); + let source_info = SourceInfo { span: self.thir[expr_id].span, scope: self.source_scope }; + // Separate path for handling branches when MC/DC is enabled. if branch_info.mcdc_state.is_some() { + let mut inject_block_marker = + |block| branch_info.inject_block_marker(&mut self.cfg, source_info, block); + let true_marker = inject_block_marker(then_block); + let false_marker = inject_block_marker(else_block); let condition_info = branch_info.fetch_mcdc_condition_info(self.tcx, true_marker, false_marker); branch_info.mcdc_branch_spans.push(MCDCBranchSpan { @@ -357,11 +375,7 @@ impl Builder<'_, '_> { return; } - branch_info.branch_spans.push(BranchSpan { - span: source_info.span, - true_marker, - false_marker, - }); + branch_info.add_two_way_branch(&mut self.cfg, source_info, then_block, else_block); } pub(crate) fn visit_coverage_branch_operation(&mut self, logical_op: LogicalOp, span: Span) { From 2b6adb06fb8307f19bd42d2fce3ad338dc6112ef Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sun, 21 Apr 2024 13:21:58 +1000 Subject: [PATCH 12/13] coverage: Separate branch pairs from other mapping kinds This clears the way for larger changes to how branches are handled by the coverage instrumentor, in order to support branch coverage for more language constructs. --- .../rustc_mir_transform/src/coverage/mod.rs | 22 +++++++---- .../rustc_mir_transform/src/coverage/spans.rs | 37 +++++++++++++------ .../src/coverage/spans/from_mir.rs | 10 ++--- 3 files changed, 44 insertions(+), 25 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index d1516605fb603..0b15c52c28143 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -9,7 +9,7 @@ mod tests; use self::counters::{CounterIncrementSite, CoverageCounters}; use self::graph::{BasicCoverageBlock, CoverageGraph}; -use self::spans::{BcbMapping, BcbMappingKind, CoverageSpans}; +use self::spans::{BcbBranchPair, BcbMapping, BcbMappingKind, CoverageSpans}; use crate::MirPass; @@ -141,14 +141,10 @@ fn create_mappings<'tcx>( let mut mappings = Vec::new(); - mappings.extend(coverage_spans.all_bcb_mappings().filter_map( + mappings.extend(coverage_spans.mappings.iter().filter_map( |BcbMapping { kind: bcb_mapping_kind, span }| { let kind = match *bcb_mapping_kind { BcbMappingKind::Code(bcb) => MappingKind::Code(term_for_bcb(bcb)), - BcbMappingKind::Branch { true_bcb, false_bcb } => MappingKind::Branch { - true_term: term_for_bcb(true_bcb), - false_term: term_for_bcb(false_bcb), - }, BcbMappingKind::MCDCBranch { true_bcb, false_bcb, condition_info: None } => { MappingKind::Branch { true_term: term_for_bcb(true_bcb), @@ -173,6 +169,16 @@ fn create_mappings<'tcx>( }, )); + mappings.extend(coverage_spans.branch_pairs.iter().filter_map( + |&BcbBranchPair { span, true_bcb, false_bcb }| { + let true_term = term_for_bcb(true_bcb); + let false_term = term_for_bcb(false_bcb); + let kind = MappingKind::Branch { true_term, false_term }; + let code_region = make_code_region(source_map, file_name, span, body_span)?; + Some(Mapping { kind, code_region }) + }, + )); + mappings } @@ -241,7 +247,7 @@ fn inject_mcdc_statements<'tcx>( // Inject test vector update first because `inject_statement` always insert new statement at head. for (end_bcbs, bitmap_idx) in - coverage_spans.all_bcb_mappings().filter_map(|mapping| match &mapping.kind { + coverage_spans.mappings.iter().filter_map(|mapping| match &mapping.kind { BcbMappingKind::MCDCDecision { end_bcbs, bitmap_idx, .. } => { Some((end_bcbs, *bitmap_idx)) } @@ -255,7 +261,7 @@ fn inject_mcdc_statements<'tcx>( } for (true_bcb, false_bcb, condition_id) in - coverage_spans.all_bcb_mappings().filter_map(|mapping| match mapping.kind { + coverage_spans.mappings.iter().filter_map(|mapping| match mapping.kind { BcbMappingKind::MCDCBranch { true_bcb, false_bcb, condition_info } => { Some((true_bcb, false_bcb, condition_info?.condition_id)) } diff --git a/compiler/rustc_mir_transform/src/coverage/spans.rs b/compiler/rustc_mir_transform/src/coverage/spans.rs index 8d2241783f647..88f18b7208574 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans.rs @@ -15,8 +15,10 @@ mod from_mir; pub(super) enum BcbMappingKind { /// Associates an ordinary executable code span with its corresponding BCB. Code(BasicCoverageBlock), - /// Associates a branch span with BCBs for its true and false arms. - Branch { true_bcb: BasicCoverageBlock, false_bcb: BasicCoverageBlock }, + + // Ordinary branch mappings are stored separately, so they don't have a + // variant in this enum. + // /// Associates a mcdc branch span with condition info besides fields for normal branch. MCDCBranch { true_bcb: BasicCoverageBlock, @@ -35,9 +37,20 @@ pub(super) struct BcbMapping { pub(super) span: Span, } +/// This is separate from [`BcbMappingKind`] to help prepare for larger changes +/// that will be needed for improved branch coverage in the future. +/// (See .) +#[derive(Debug)] +pub(super) struct BcbBranchPair { + pub(super) span: Span, + pub(super) true_bcb: BasicCoverageBlock, + pub(super) false_bcb: BasicCoverageBlock, +} + pub(super) struct CoverageSpans { bcb_has_mappings: BitSet, - mappings: Vec, + pub(super) mappings: Vec, + pub(super) branch_pairs: Vec, test_vector_bitmap_bytes: u32, } @@ -46,10 +59,6 @@ impl CoverageSpans { self.bcb_has_mappings.contains(bcb) } - pub(super) fn all_bcb_mappings(&self) -> impl Iterator { - self.mappings.iter() - } - pub(super) fn test_vector_bitmap_bytes(&self) -> u32 { self.test_vector_bitmap_bytes } @@ -65,6 +74,7 @@ pub(super) fn generate_coverage_spans( basic_coverage_blocks: &CoverageGraph, ) -> Option { let mut mappings = vec![]; + let mut branch_pairs = vec![]; if hir_info.is_async_fn { // An async function desugars into a function that returns a future, @@ -86,7 +96,7 @@ pub(super) fn generate_coverage_spans( BcbMapping { kind: BcbMappingKind::Code(bcb), span } })); - mappings.extend(from_mir::extract_branch_mappings( + branch_pairs.extend(from_mir::extract_branch_pairs( mir_body, hir_info, basic_coverage_blocks, @@ -99,7 +109,7 @@ pub(super) fn generate_coverage_spans( )); } - if mappings.is_empty() { + if mappings.is_empty() && branch_pairs.is_empty() { return None; } @@ -112,8 +122,7 @@ pub(super) fn generate_coverage_spans( for BcbMapping { kind, span: _ } in &mappings { match *kind { BcbMappingKind::Code(bcb) => insert(bcb), - BcbMappingKind::Branch { true_bcb, false_bcb } - | BcbMappingKind::MCDCBranch { true_bcb, false_bcb, .. } => { + BcbMappingKind::MCDCBranch { true_bcb, false_bcb, .. } => { insert(true_bcb); insert(false_bcb); } @@ -126,8 +135,12 @@ pub(super) fn generate_coverage_spans( } } } + for &BcbBranchPair { true_bcb, false_bcb, .. } in &branch_pairs { + insert(true_bcb); + insert(false_bcb); + } - Some(CoverageSpans { bcb_has_mappings, mappings, test_vector_bitmap_bytes }) + Some(CoverageSpans { bcb_has_mappings, mappings, branch_pairs, test_vector_bitmap_bytes }) } #[derive(Debug)] diff --git a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs index 3c64591d43e9c..64f21d74b1cd5 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs @@ -13,7 +13,7 @@ use rustc_span::{ExpnKind, MacroKind, Span, Symbol}; use crate::coverage::graph::{ BasicCoverageBlock, BasicCoverageBlockData, CoverageGraph, START_BCB, }; -use crate::coverage::spans::{BcbMapping, BcbMappingKind}; +use crate::coverage::spans::{BcbBranchPair, BcbMapping, BcbMappingKind}; use crate::coverage::ExtractedHirInfo; /// Traverses the MIR body to produce an initial collection of coverage-relevant @@ -388,16 +388,16 @@ fn resolve_block_markers( } // FIXME: There is currently a lot of redundancy between -// `extract_branch_mappings` and `extract_mcdc_mappings`. This is needed so +// `extract_branch_pairs` and `extract_mcdc_mappings`. This is needed so // that they can each be modified without interfering with the other, but in // the long term we should try to bring them together again when branch coverage // and MC/DC coverage support are more mature. -pub(super) fn extract_branch_mappings( +pub(super) fn extract_branch_pairs( mir_body: &mir::Body<'_>, hir_info: &ExtractedHirInfo, basic_coverage_blocks: &CoverageGraph, -) -> Vec { +) -> Vec { let Some(branch_info) = mir_body.coverage_branch_info.as_deref() else { return vec![] }; let block_markers = resolve_block_markers(branch_info, mir_body); @@ -420,7 +420,7 @@ pub(super) fn extract_branch_mappings( let true_bcb = bcb_from_marker(true_marker)?; let false_bcb = bcb_from_marker(false_marker)?; - Some(BcbMapping { kind: BcbMappingKind::Branch { true_bcb, false_bcb }, span }) + Some(BcbBranchPair { span, true_bcb, false_bcb }) }) .collect::>() } From 6146a51f17f19b557c2baf11a1ae04cbafdd89bb Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Mon, 22 Apr 2024 14:54:28 +0200 Subject: [PATCH 13/13] Add more context to the forbidden dep-graph read ICE error message. --- compiler/rustc_query_system/src/dep_graph/graph.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index af70e2a426423..2b3fa7f6cfad3 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -1402,6 +1402,10 @@ fn panic_on_forbidden_read(data: &DepGraphData, dep_node_index: DepN panic!( "Error: trying to record dependency on DepNode {dep_node} in a \ - context that does not allow it (e.g. during query deserialization)." + context that does not allow it (e.g. during query deserialization). \ + The most common case of recording a dependency on a DepNode `foo` is \ + when the correspondng query `foo` is invoked. Invoking queries is not \ + allowed as part of loading something from the incremental on-disk cache. \ + See ." ) }