From b0eb55a092506aa4cfe34969c56adfeca9616750 Mon Sep 17 00:00:00 2001 From: jumbatm <30644300+jumbatm@users.noreply.github.com> Date: Sat, 15 Aug 2020 13:18:54 +1000 Subject: [PATCH 01/10] Add test demonstrating the issue. --- .../ui/lint/clashing-extern-fn-recursion.rs | 117 ++++++++++++++++++ 1 file changed, 117 insertions(+) create mode 100644 src/test/ui/lint/clashing-extern-fn-recursion.rs diff --git a/src/test/ui/lint/clashing-extern-fn-recursion.rs b/src/test/ui/lint/clashing-extern-fn-recursion.rs new file mode 100644 index 0000000000000..51bfd2032838c --- /dev/null +++ b/src/test/ui/lint/clashing-extern-fn-recursion.rs @@ -0,0 +1,117 @@ +// check-pass +// +// This tests checks that clashing_extern_declarations handles types that are recursive through a +// pointer or ref argument. See #75512. + +#![crate_type = "lib"] + +mod raw_ptr_recursion { + mod a { + #[repr(C)] + struct Pointy { + pointy: *const Pointy, + } + + extern "C" { + fn run_pointy(pointy: Pointy); + } + } + mod b { + #[repr(C)] + struct Pointy { + pointy: *const Pointy, + } + + extern "C" { + fn run_pointy(pointy: Pointy); + } + } +} + +mod raw_ptr_recursion_once_removed { + mod a { + #[repr(C)] + struct Pointy1 { + pointy_two: *const Pointy2, + } + + #[repr(C)] + struct Pointy2 { + pointy_one: *const Pointy1, + } + + extern "C" { + fn run_pointy2(pointy: Pointy2); + } + } + + mod b { + #[repr(C)] + struct Pointy1 { + pointy_two: *const Pointy2, + } + + #[repr(C)] + struct Pointy2 { + pointy_one: *const Pointy1, + } + + extern "C" { + fn run_pointy2(pointy: Pointy2); + } + } +} + +mod ref_recursion { + mod a { + #[repr(C)] + struct Reffy<'a> { + reffy: &'a Reffy<'a>, + } + + extern "C" { + fn reffy_recursion(reffy: Reffy); + } + } + mod b { + #[repr(C)] + struct Reffy<'a> { + reffy: &'a Reffy<'a>, + } + + extern "C" { + fn reffy_recursion(reffy: Reffy); + } + } +} + +mod ref_recursion_once_removed { + mod a { + #[repr(C)] + struct Reffy1<'a> { + reffy: &'a Reffy1<'a>, + } + + struct Reffy2<'a> { + reffy: &'a Reffy2<'a>, + } + + extern "C" { + fn reffy_once_removed(reffy: Reffy1); + } + } + mod b { + #[repr(C)] + struct Reffy1<'a> { + reffy: &'a Reffy1<'a>, + } + + struct Reffy2<'a> { + reffy: &'a Reffy2<'a>, + } + + extern "C" { + fn reffy_once_removed(reffy: Reffy1); + } + } +} From db753137a14b04886567a1f20768b96feba05594 Mon Sep 17 00:00:00 2001 From: jumbatm <30644300+jumbatm@users.noreply.github.com> Date: Sat, 15 Aug 2020 15:05:18 +1000 Subject: [PATCH 02/10] Fix stack overflow for recursive types. Adds a seen set to structurally_same_type to avoid recursing indefinitely when a reference or pointer member introduces a cycle in the visited types. --- src/librustc_lint/builtin.rs | 289 ++++++++++++++++++++++------------- 1 file changed, 187 insertions(+), 102 deletions(-) diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs index 3859d0f163ad5..0e578ac5034db 100644 --- a/src/librustc_lint/builtin.rs +++ b/src/librustc_lint/builtin.rs @@ -2153,44 +2153,99 @@ impl ClashingExternDeclarations { b: Ty<'tcx>, ckind: CItemKind, ) -> bool { - debug!("structurally_same_type(cx, a = {:?}, b = {:?})", a, b); - let tcx = cx.tcx; - if a == b || rustc_middle::ty::TyS::same_type(a, b) { - // All nominally-same types are structurally same, too. - true - } else { - // Do a full, depth-first comparison between the two. - use rustc_middle::ty::TyKind::*; - let a_kind = &a.kind; - let b_kind = &b.kind; - - let compare_layouts = |a, b| -> bool { - let a_layout = &cx.layout_of(a).unwrap().layout.abi; - let b_layout = &cx.layout_of(b).unwrap().layout.abi; - debug!("{:?} == {:?} = {}", a_layout, b_layout, a_layout == b_layout); - a_layout == b_layout - }; + // In order to avoid endlessly recursing on recursive types, we maintain a "seen" set. + // We'll need to store every combination of types we encounter anyway, so we also memoize + // the result. + struct SeenSet<'tcx>(FxHashMap<(Ty<'tcx>, Ty<'tcx>), Option>); + + enum SeenSetResult { + /// We've never seen this combination of types. + Unseen, + /// We've seen this combination of types, but are still computing the result. + Computing, + /// We've seen this combination of types, and have already computed the result. + Computed(bool), + } + + impl<'tcx> SeenSet<'tcx> { + fn new() -> Self { + SeenSet(FxHashMap::default()) + } + /// Mark (a, b) as `Computing`. + fn mark_computing(&mut self, a: Ty<'tcx>, b: Ty<'tcx>) { + self.0.insert((a, b), None); + } + /// Mark (a, b) as `Computed(result)`. + fn mark_computed(&mut self, a: Ty<'tcx>, b: Ty<'tcx>, result: bool) { + *self.0.get_mut(&(a, b)).expect("Missing prior call to mark_computing") = + Some(result); + } + fn get(&self, a: Ty<'tcx>, b: Ty<'tcx>) -> SeenSetResult { + match self.0.get(&(a, b)) { + None => SeenSetResult::Unseen, + Some(None) => SeenSetResult::Computing, + Some(Some(b)) => SeenSetResult::Computed(*b), + } + } + } + fn structurally_same_type_impl<'tcx>( + seen_types: &mut SeenSet<'tcx>, + cx: &LateContext<'tcx>, + a: Ty<'tcx>, + b: Ty<'tcx>, + ckind: CItemKind, + ) -> bool { + debug!("structurally_same_type_impl(cx, a = {:?}, b = {:?})", a, b); + match seen_types.get(a, b) { + // If we've already computed the result, just return the memoized result. + SeenSetResult::Computed(result) => result, + // We are already in the process of computing structural sameness for this type, + // meaning we've found a cycle. The types are structurally same, then. + SeenSetResult::Computing => true, + // We haven't seen this combination of types at all -- compute their sameness. + SeenSetResult::Unseen => { + seen_types.mark_computing(a, b); + let tcx = cx.tcx; + let result = if a == b || rustc_middle::ty::TyS::same_type(a, b) { + // All nominally-same types are structurally same, too. + true + } else { + // Do a full, depth-first comparison between the two. + use rustc_middle::ty::TyKind::*; + let a_kind = &a.kind; + let b_kind = &b.kind; + + let compare_layouts = |a, b| -> bool { + let a_layout = &cx.layout_of(a).unwrap().layout.abi; + let b_layout = &cx.layout_of(b).unwrap().layout.abi; + debug!("{:?} == {:?} = {}", a_layout, b_layout, a_layout == b_layout); + a_layout == b_layout + }; + + #[allow(rustc::usage_of_ty_tykind)] + let is_primitive_or_pointer = |kind: &ty::TyKind<'_>| { + kind.is_primitive() || matches!(kind, RawPtr(..)) + }; - #[allow(rustc::usage_of_ty_tykind)] - let is_primitive_or_pointer = - |kind: &ty::TyKind<'_>| kind.is_primitive() || matches!(kind, RawPtr(..)); - - match (a_kind, b_kind) { - (Adt(_, a_substs), Adt(_, b_substs)) => { - let a = a.subst(cx.tcx, a_substs); - let b = b.subst(cx.tcx, b_substs); - debug!("Comparing {:?} and {:?}", a, b); - - if let (Adt(a_def, ..), Adt(b_def, ..)) = (&a.kind, &b.kind) { - // Grab a flattened representation of all fields. - let a_fields = a_def.variants.iter().flat_map(|v| v.fields.iter()); - let b_fields = b_def.variants.iter().flat_map(|v| v.fields.iter()); - compare_layouts(a, b) + match (a_kind, b_kind) { + (Adt(_, a_substs), Adt(_, b_substs)) => { + let a = a.subst(cx.tcx, a_substs); + let b = b.subst(cx.tcx, b_substs); + debug!("Comparing {:?} and {:?}", a, b); + + if let (Adt(a_def, ..), Adt(b_def, ..)) = (&a.kind, &b.kind) { + // Grab a flattened representation of all fields. + let a_fields = + a_def.variants.iter().flat_map(|v| v.fields.iter()); + let b_fields = + b_def.variants.iter().flat_map(|v| v.fields.iter()); + compare_layouts(a, b) && a_fields.eq_by( b_fields, |&ty::FieldDef { did: a_did, .. }, &ty::FieldDef { did: b_did, .. }| { - Self::structurally_same_type( + structurally_same_type_impl( + seen_types, cx, tcx.type_of(a_did), tcx.type_of(b_did), @@ -2198,78 +2253,108 @@ impl ClashingExternDeclarations { ) }, ) - } else { - unreachable!() - } - } - (Array(a_ty, a_const), Array(b_ty, b_const)) => { - // For arrays, we also check the constness of the type. - a_const.val == b_const.val - && Self::structurally_same_type(cx, a_const.ty, b_const.ty, ckind) - && Self::structurally_same_type(cx, a_ty, b_ty, ckind) - } - (Slice(a_ty), Slice(b_ty)) => Self::structurally_same_type(cx, a_ty, b_ty, ckind), - (RawPtr(a_tymut), RawPtr(b_tymut)) => { - a_tymut.mutbl == b_tymut.mutbl - && Self::structurally_same_type(cx, &a_tymut.ty, &b_tymut.ty, ckind) - } - (Ref(_a_region, a_ty, a_mut), Ref(_b_region, b_ty, b_mut)) => { - // For structural sameness, we don't need the region to be same. - a_mut == b_mut && Self::structurally_same_type(cx, a_ty, b_ty, ckind) - } - (FnDef(..), FnDef(..)) => { - let a_poly_sig = a.fn_sig(tcx); - let b_poly_sig = b.fn_sig(tcx); - - // As we don't compare regions, skip_binder is fine. - let a_sig = a_poly_sig.skip_binder(); - let b_sig = b_poly_sig.skip_binder(); - - (a_sig.abi, a_sig.unsafety, a_sig.c_variadic) - == (b_sig.abi, b_sig.unsafety, b_sig.c_variadic) - && a_sig.inputs().iter().eq_by(b_sig.inputs().iter(), |a, b| { - Self::structurally_same_type(cx, a, b, ckind) - }) - && Self::structurally_same_type(cx, a_sig.output(), b_sig.output(), ckind) - } - (Tuple(a_substs), Tuple(b_substs)) => { - a_substs.types().eq_by(b_substs.types(), |a_ty, b_ty| { - Self::structurally_same_type(cx, a_ty, b_ty, ckind) - }) - } - // For these, it's not quite as easy to define structural-sameness quite so easily. - // For the purposes of this lint, take the conservative approach and mark them as - // not structurally same. - (Dynamic(..), Dynamic(..)) - | (Error(..), Error(..)) - | (Closure(..), Closure(..)) - | (Generator(..), Generator(..)) - | (GeneratorWitness(..), GeneratorWitness(..)) - | (Projection(..), Projection(..)) - | (Opaque(..), Opaque(..)) => false, - - // These definitely should have been caught above. - (Bool, Bool) | (Char, Char) | (Never, Never) | (Str, Str) => unreachable!(), - - // An Adt and a primitive type. This can be FFI-safe is the ADT is an enum with a - // non-null field. - (Adt(..), other_kind) | (other_kind, Adt(..)) - if is_primitive_or_pointer(other_kind) => - { - let (primitive, adt) = - if is_primitive_or_pointer(&a.kind) { (a, b) } else { (b, a) }; - if let Some(ty) = crate::types::repr_nullable_ptr(cx, adt, ckind) { - ty == primitive - } else { - compare_layouts(a, b) - } + } else { + unreachable!() + } + } + (Array(a_ty, a_const), Array(b_ty, b_const)) => { + // For arrays, we also check the constness of the type. + a_const.val == b_const.val + && structurally_same_type_impl( + seen_types, cx, a_const.ty, b_const.ty, ckind, + ) + && structurally_same_type_impl( + seen_types, cx, a_ty, b_ty, ckind, + ) + } + (Slice(a_ty), Slice(b_ty)) => { + structurally_same_type_impl(seen_types, cx, a_ty, b_ty, ckind) + } + (RawPtr(a_tymut), RawPtr(b_tymut)) => { + a_tymut.mutbl == b_tymut.mutbl + && structurally_same_type_impl( + seen_types, + cx, + &a_tymut.ty, + &b_tymut.ty, + ckind, + ) + } + (Ref(_a_region, a_ty, a_mut), Ref(_b_region, b_ty, b_mut)) => { + // For structural sameness, we don't need the region to be same. + a_mut == b_mut + && structurally_same_type_impl( + seen_types, cx, a_ty, b_ty, ckind, + ) + } + (FnDef(..), FnDef(..)) => { + let a_poly_sig = a.fn_sig(tcx); + let b_poly_sig = b.fn_sig(tcx); + + // As we don't compare regions, skip_binder is fine. + let a_sig = a_poly_sig.skip_binder(); + let b_sig = b_poly_sig.skip_binder(); + + (a_sig.abi, a_sig.unsafety, a_sig.c_variadic) + == (b_sig.abi, b_sig.unsafety, b_sig.c_variadic) + && a_sig.inputs().iter().eq_by(b_sig.inputs().iter(), |a, b| { + structurally_same_type_impl(seen_types, cx, a, b, ckind) + }) + && structurally_same_type_impl( + seen_types, + cx, + a_sig.output(), + b_sig.output(), + ckind, + ) + } + (Tuple(a_substs), Tuple(b_substs)) => { + a_substs.types().eq_by(b_substs.types(), |a_ty, b_ty| { + structurally_same_type_impl(seen_types, cx, a_ty, b_ty, ckind) + }) + } + // For these, it's not quite as easy to define structural-sameness quite so easily. + // For the purposes of this lint, take the conservative approach and mark them as + // not structurally same. + (Dynamic(..), Dynamic(..)) + | (Error(..), Error(..)) + | (Closure(..), Closure(..)) + | (Generator(..), Generator(..)) + | (GeneratorWitness(..), GeneratorWitness(..)) + | (Projection(..), Projection(..)) + | (Opaque(..), Opaque(..)) => false, + + // These definitely should have been caught above. + (Bool, Bool) | (Char, Char) | (Never, Never) | (Str, Str) => { + unreachable!() + } + + // An Adt and a primitive type. This can be FFI-safe is the ADT is an enum with a + // non-null field. + (Adt(..), other_kind) | (other_kind, Adt(..)) + if is_primitive_or_pointer(other_kind) => + { + let (primitive, adt) = + if is_primitive_or_pointer(&a.kind) { (a, b) } else { (b, a) }; + if let Some(ty) = crate::types::repr_nullable_ptr(cx, adt, ckind) { + ty == primitive + } else { + compare_layouts(a, b) + } + } + // Otherwise, just compare the layouts. This may fail to lint for some + // incompatible types, but at the very least, will stop reads into + // uninitialised memory. + _ => compare_layouts(a, b), + } + }; + seen_types.mark_computed(a, b, result); + result } - // Otherwise, just compare the layouts. This may fail to lint for some - // incompatible types, but at the very least, will stop reads into - // uninitialised memory. - _ => compare_layouts(a, b), } } + let mut seen_types = SeenSet::new(); + structurally_same_type_impl(&mut seen_types, cx, a, b, ckind) } } From 154b74e8f959581d37d1a1ae860ee1bf7b682f34 Mon Sep 17 00:00:00 2001 From: jumbatm <30644300+jumbatm@users.noreply.github.com> Date: Sun, 16 Aug 2020 10:26:45 +1000 Subject: [PATCH 03/10] Actually introduce a cycle in Reffy test. --- src/test/ui/lint/clashing-extern-fn-recursion.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/test/ui/lint/clashing-extern-fn-recursion.rs b/src/test/ui/lint/clashing-extern-fn-recursion.rs index 51bfd2032838c..ab0fd0a2e7085 100644 --- a/src/test/ui/lint/clashing-extern-fn-recursion.rs +++ b/src/test/ui/lint/clashing-extern-fn-recursion.rs @@ -89,28 +89,30 @@ mod ref_recursion_once_removed { mod a { #[repr(C)] struct Reffy1<'a> { - reffy: &'a Reffy1<'a>, + reffy: &'a Reffy2<'a>, } struct Reffy2<'a> { - reffy: &'a Reffy2<'a>, + reffy: &'a Reffy1<'a>, } extern "C" { + #[allow(improper_ctypes)] fn reffy_once_removed(reffy: Reffy1); } } mod b { #[repr(C)] struct Reffy1<'a> { - reffy: &'a Reffy1<'a>, + reffy: &'a Reffy2<'a>, } struct Reffy2<'a> { - reffy: &'a Reffy2<'a>, + reffy: &'a Reffy1<'a>, } extern "C" { + #[allow(improper_ctypes)] fn reffy_once_removed(reffy: Reffy1); } } From a1fa4e05acb2cbda9a02cc34f8000c965328dbde Mon Sep 17 00:00:00 2001 From: jumbatm <30644300+jumbatm@users.noreply.github.com> Date: Sun, 16 Aug 2020 10:41:59 +1000 Subject: [PATCH 04/10] Remove unnecessary rebinding of def ids. --- src/librustc_lint/builtin.rs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs index 0e578ac5034db..47f680932f14e 100644 --- a/src/librustc_lint/builtin.rs +++ b/src/librustc_lint/builtin.rs @@ -2228,18 +2228,15 @@ impl ClashingExternDeclarations { }; match (a_kind, b_kind) { - (Adt(_, a_substs), Adt(_, b_substs)) => { + (Adt(a_def, a_substs), Adt(b_def, b_substs)) => { let a = a.subst(cx.tcx, a_substs); let b = b.subst(cx.tcx, b_substs); debug!("Comparing {:?} and {:?}", a, b); - if let (Adt(a_def, ..), Adt(b_def, ..)) = (&a.kind, &b.kind) { - // Grab a flattened representation of all fields. - let a_fields = - a_def.variants.iter().flat_map(|v| v.fields.iter()); - let b_fields = - b_def.variants.iter().flat_map(|v| v.fields.iter()); - compare_layouts(a, b) + // Grab a flattened representation of all fields. + let a_fields = a_def.variants.iter().flat_map(|v| v.fields.iter()); + let b_fields = b_def.variants.iter().flat_map(|v| v.fields.iter()); + compare_layouts(a, b) && a_fields.eq_by( b_fields, |&ty::FieldDef { did: a_did, .. }, @@ -2253,9 +2250,6 @@ impl ClashingExternDeclarations { ) }, ) - } else { - unreachable!() - } } (Array(a_ty, a_const), Array(b_ty, b_const)) => { // For arrays, we also check the constness of the type. From 80c2c80d52e2384ee6ce33a7b41c0c47c4f0ffd1 Mon Sep 17 00:00:00 2001 From: jumbatm <30644300+jumbatm@users.noreply.github.com> Date: Sun, 16 Aug 2020 10:47:35 +1000 Subject: [PATCH 05/10] Remove structural equiv check for Array const. --- src/librustc_lint/builtin.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs index 47f680932f14e..2cf0ed390f4fc 100644 --- a/src/librustc_lint/builtin.rs +++ b/src/librustc_lint/builtin.rs @@ -2254,9 +2254,6 @@ impl ClashingExternDeclarations { (Array(a_ty, a_const), Array(b_ty, b_const)) => { // For arrays, we also check the constness of the type. a_const.val == b_const.val - && structurally_same_type_impl( - seen_types, cx, a_const.ty, b_const.ty, ckind, - ) && structurally_same_type_impl( seen_types, cx, a_ty, b_ty, ckind, ) From bca48ad7acbc7b7462bce58f0513c02c4e5444b5 Mon Sep 17 00:00:00 2001 From: jumbatm <30644300+jumbatm@users.noreply.github.com> Date: Sun, 16 Aug 2020 10:50:20 +1000 Subject: [PATCH 06/10] Reduce indentation by replacing match arm w/ early return. --- src/librustc_lint/builtin.rs | 243 +++++++++++++++++------------------ 1 file changed, 118 insertions(+), 125 deletions(-) diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs index 2cf0ed390f4fc..c23b443645de6 100644 --- a/src/librustc_lint/builtin.rs +++ b/src/librustc_lint/builtin.rs @@ -2198,45 +2198,46 @@ impl ClashingExternDeclarations { debug!("structurally_same_type_impl(cx, a = {:?}, b = {:?})", a, b); match seen_types.get(a, b) { // If we've already computed the result, just return the memoized result. - SeenSetResult::Computed(result) => result, + SeenSetResult::Computed(result) => return result, // We are already in the process of computing structural sameness for this type, // meaning we've found a cycle. The types are structurally same, then. - SeenSetResult::Computing => true, - // We haven't seen this combination of types at all -- compute their sameness. - SeenSetResult::Unseen => { - seen_types.mark_computing(a, b); - let tcx = cx.tcx; - let result = if a == b || rustc_middle::ty::TyS::same_type(a, b) { - // All nominally-same types are structurally same, too. - true - } else { - // Do a full, depth-first comparison between the two. - use rustc_middle::ty::TyKind::*; - let a_kind = &a.kind; - let b_kind = &b.kind; - - let compare_layouts = |a, b| -> bool { - let a_layout = &cx.layout_of(a).unwrap().layout.abi; - let b_layout = &cx.layout_of(b).unwrap().layout.abi; - debug!("{:?} == {:?} = {}", a_layout, b_layout, a_layout == b_layout); - a_layout == b_layout - }; + SeenSetResult::Computing => return true, + // We haven't seen this combination of types at all -- continue on to computing + // their sameness. + SeenSetResult::Unseen => (), + } + seen_types.mark_computing(a, b); + let tcx = cx.tcx; + let result = if a == b || rustc_middle::ty::TyS::same_type(a, b) { + // All nominally-same types are structurally same, too. + true + } else { + // Do a full, depth-first comparison between the two. + use rustc_middle::ty::TyKind::*; + let a_kind = &a.kind; + let b_kind = &b.kind; + + let compare_layouts = |a, b| -> bool { + let a_layout = &cx.layout_of(a).unwrap().layout.abi; + let b_layout = &cx.layout_of(b).unwrap().layout.abi; + debug!("{:?} == {:?} = {}", a_layout, b_layout, a_layout == b_layout); + a_layout == b_layout + }; - #[allow(rustc::usage_of_ty_tykind)] - let is_primitive_or_pointer = |kind: &ty::TyKind<'_>| { - kind.is_primitive() || matches!(kind, RawPtr(..)) - }; + #[allow(rustc::usage_of_ty_tykind)] + let is_primitive_or_pointer = + |kind: &ty::TyKind<'_>| kind.is_primitive() || matches!(kind, RawPtr(..)); - match (a_kind, b_kind) { - (Adt(a_def, a_substs), Adt(b_def, b_substs)) => { - let a = a.subst(cx.tcx, a_substs); - let b = b.subst(cx.tcx, b_substs); - debug!("Comparing {:?} and {:?}", a, b); + match (a_kind, b_kind) { + (Adt(a_def, a_substs), Adt(b_def, b_substs)) => { + let a = a.subst(cx.tcx, a_substs); + let b = b.subst(cx.tcx, b_substs); + debug!("Comparing {:?} and {:?}", a, b); - // Grab a flattened representation of all fields. - let a_fields = a_def.variants.iter().flat_map(|v| v.fields.iter()); - let b_fields = b_def.variants.iter().flat_map(|v| v.fields.iter()); - compare_layouts(a, b) + // Grab a flattened representation of all fields. + let a_fields = a_def.variants.iter().flat_map(|v| v.fields.iter()); + let b_fields = b_def.variants.iter().flat_map(|v| v.fields.iter()); + compare_layouts(a, b) && a_fields.eq_by( b_fields, |&ty::FieldDef { did: a_did, .. }, @@ -2250,99 +2251,91 @@ impl ClashingExternDeclarations { ) }, ) - } - (Array(a_ty, a_const), Array(b_ty, b_const)) => { - // For arrays, we also check the constness of the type. - a_const.val == b_const.val - && structurally_same_type_impl( - seen_types, cx, a_ty, b_ty, ckind, - ) - } - (Slice(a_ty), Slice(b_ty)) => { - structurally_same_type_impl(seen_types, cx, a_ty, b_ty, ckind) - } - (RawPtr(a_tymut), RawPtr(b_tymut)) => { - a_tymut.mutbl == b_tymut.mutbl - && structurally_same_type_impl( - seen_types, - cx, - &a_tymut.ty, - &b_tymut.ty, - ckind, - ) - } - (Ref(_a_region, a_ty, a_mut), Ref(_b_region, b_ty, b_mut)) => { - // For structural sameness, we don't need the region to be same. - a_mut == b_mut - && structurally_same_type_impl( - seen_types, cx, a_ty, b_ty, ckind, - ) - } - (FnDef(..), FnDef(..)) => { - let a_poly_sig = a.fn_sig(tcx); - let b_poly_sig = b.fn_sig(tcx); - - // As we don't compare regions, skip_binder is fine. - let a_sig = a_poly_sig.skip_binder(); - let b_sig = b_poly_sig.skip_binder(); - - (a_sig.abi, a_sig.unsafety, a_sig.c_variadic) - == (b_sig.abi, b_sig.unsafety, b_sig.c_variadic) - && a_sig.inputs().iter().eq_by(b_sig.inputs().iter(), |a, b| { - structurally_same_type_impl(seen_types, cx, a, b, ckind) - }) - && structurally_same_type_impl( - seen_types, - cx, - a_sig.output(), - b_sig.output(), - ckind, - ) - } - (Tuple(a_substs), Tuple(b_substs)) => { - a_substs.types().eq_by(b_substs.types(), |a_ty, b_ty| { - structurally_same_type_impl(seen_types, cx, a_ty, b_ty, ckind) - }) - } - // For these, it's not quite as easy to define structural-sameness quite so easily. - // For the purposes of this lint, take the conservative approach and mark them as - // not structurally same. - (Dynamic(..), Dynamic(..)) - | (Error(..), Error(..)) - | (Closure(..), Closure(..)) - | (Generator(..), Generator(..)) - | (GeneratorWitness(..), GeneratorWitness(..)) - | (Projection(..), Projection(..)) - | (Opaque(..), Opaque(..)) => false, - - // These definitely should have been caught above. - (Bool, Bool) | (Char, Char) | (Never, Never) | (Str, Str) => { - unreachable!() - } - - // An Adt and a primitive type. This can be FFI-safe is the ADT is an enum with a - // non-null field. - (Adt(..), other_kind) | (other_kind, Adt(..)) - if is_primitive_or_pointer(other_kind) => - { - let (primitive, adt) = - if is_primitive_or_pointer(&a.kind) { (a, b) } else { (b, a) }; - if let Some(ty) = crate::types::repr_nullable_ptr(cx, adt, ckind) { - ty == primitive - } else { - compare_layouts(a, b) - } - } - // Otherwise, just compare the layouts. This may fail to lint for some - // incompatible types, but at the very least, will stop reads into - // uninitialised memory. - _ => compare_layouts(a, b), + } + (Array(a_ty, a_const), Array(b_ty, b_const)) => { + // For arrays, we also check the constness of the type. + a_const.val == b_const.val + && structurally_same_type_impl(seen_types, cx, a_ty, b_ty, ckind) + } + (Slice(a_ty), Slice(b_ty)) => { + structurally_same_type_impl(seen_types, cx, a_ty, b_ty, ckind) + } + (RawPtr(a_tymut), RawPtr(b_tymut)) => { + a_tymut.mutbl == b_tymut.mutbl + && structurally_same_type_impl( + seen_types, + cx, + &a_tymut.ty, + &b_tymut.ty, + ckind, + ) + } + (Ref(_a_region, a_ty, a_mut), Ref(_b_region, b_ty, b_mut)) => { + // For structural sameness, we don't need the region to be same. + a_mut == b_mut + && structurally_same_type_impl(seen_types, cx, a_ty, b_ty, ckind) + } + (FnDef(..), FnDef(..)) => { + let a_poly_sig = a.fn_sig(tcx); + let b_poly_sig = b.fn_sig(tcx); + + // As we don't compare regions, skip_binder is fine. + let a_sig = a_poly_sig.skip_binder(); + let b_sig = b_poly_sig.skip_binder(); + + (a_sig.abi, a_sig.unsafety, a_sig.c_variadic) + == (b_sig.abi, b_sig.unsafety, b_sig.c_variadic) + && a_sig.inputs().iter().eq_by(b_sig.inputs().iter(), |a, b| { + structurally_same_type_impl(seen_types, cx, a, b, ckind) + }) + && structurally_same_type_impl( + seen_types, + cx, + a_sig.output(), + b_sig.output(), + ckind, + ) + } + (Tuple(a_substs), Tuple(b_substs)) => { + a_substs.types().eq_by(b_substs.types(), |a_ty, b_ty| { + structurally_same_type_impl(seen_types, cx, a_ty, b_ty, ckind) + }) + } + // For these, it's not quite as easy to define structural-sameness quite so easily. + // For the purposes of this lint, take the conservative approach and mark them as + // not structurally same. + (Dynamic(..), Dynamic(..)) + | (Error(..), Error(..)) + | (Closure(..), Closure(..)) + | (Generator(..), Generator(..)) + | (GeneratorWitness(..), GeneratorWitness(..)) + | (Projection(..), Projection(..)) + | (Opaque(..), Opaque(..)) => false, + + // These definitely should have been caught above. + (Bool, Bool) | (Char, Char) | (Never, Never) | (Str, Str) => unreachable!(), + + // An Adt and a primitive type. This can be FFI-safe is the ADT is an enum with a + // non-null field. + (Adt(..), other_kind) | (other_kind, Adt(..)) + if is_primitive_or_pointer(other_kind) => + { + let (primitive, adt) = + if is_primitive_or_pointer(&a.kind) { (a, b) } else { (b, a) }; + if let Some(ty) = crate::types::repr_nullable_ptr(cx, adt, ckind) { + ty == primitive + } else { + compare_layouts(a, b) } - }; - seen_types.mark_computed(a, b, result); - result + } + // Otherwise, just compare the layouts. This may fail to lint for some + // incompatible types, but at the very least, will stop reads into + // uninitialised memory. + _ => compare_layouts(a, b), } - } + }; + seen_types.mark_computed(a, b, result); + result } let mut seen_types = SeenSet::new(); structurally_same_type_impl(&mut seen_types, cx, a, b, ckind) From 6c57de1166d36725a689cef17e0dab8b9abcd00b Mon Sep 17 00:00:00 2001 From: jumbatm <30644300+jumbatm@users.noreply.github.com> Date: Sun, 16 Aug 2020 11:01:14 +1000 Subject: [PATCH 07/10] Don't memoize seen types. That cache is unlikely to be particularly useful within a single invocation of structurally_same_type, especially compared to memoizing results across _all_ invocations of that function. --- src/librustc_lint/builtin.rs | 60 ++++++------------------------------ 1 file changed, 9 insertions(+), 51 deletions(-) diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs index c23b443645de6..781ebb6716758 100644 --- a/src/librustc_lint/builtin.rs +++ b/src/librustc_lint/builtin.rs @@ -2153,62 +2153,22 @@ impl ClashingExternDeclarations { b: Ty<'tcx>, ckind: CItemKind, ) -> bool { - // In order to avoid endlessly recursing on recursive types, we maintain a "seen" set. - // We'll need to store every combination of types we encounter anyway, so we also memoize - // the result. - struct SeenSet<'tcx>(FxHashMap<(Ty<'tcx>, Ty<'tcx>), Option>); - - enum SeenSetResult { - /// We've never seen this combination of types. - Unseen, - /// We've seen this combination of types, but are still computing the result. - Computing, - /// We've seen this combination of types, and have already computed the result. - Computed(bool), - } - - impl<'tcx> SeenSet<'tcx> { - fn new() -> Self { - SeenSet(FxHashMap::default()) - } - /// Mark (a, b) as `Computing`. - fn mark_computing(&mut self, a: Ty<'tcx>, b: Ty<'tcx>) { - self.0.insert((a, b), None); - } - /// Mark (a, b) as `Computed(result)`. - fn mark_computed(&mut self, a: Ty<'tcx>, b: Ty<'tcx>, result: bool) { - *self.0.get_mut(&(a, b)).expect("Missing prior call to mark_computing") = - Some(result); - } - fn get(&self, a: Ty<'tcx>, b: Ty<'tcx>) -> SeenSetResult { - match self.0.get(&(a, b)) { - None => SeenSetResult::Unseen, - Some(None) => SeenSetResult::Computing, - Some(Some(b)) => SeenSetResult::Computed(*b), - } - } - } fn structurally_same_type_impl<'tcx>( - seen_types: &mut SeenSet<'tcx>, + seen_types: &mut FxHashSet<(Ty<'tcx>, Ty<'tcx>)>, cx: &LateContext<'tcx>, a: Ty<'tcx>, b: Ty<'tcx>, ckind: CItemKind, ) -> bool { debug!("structurally_same_type_impl(cx, a = {:?}, b = {:?})", a, b); - match seen_types.get(a, b) { - // If we've already computed the result, just return the memoized result. - SeenSetResult::Computed(result) => return result, - // We are already in the process of computing structural sameness for this type, - // meaning we've found a cycle. The types are structurally same, then. - SeenSetResult::Computing => return true, - // We haven't seen this combination of types at all -- continue on to computing - // their sameness. - SeenSetResult::Unseen => (), + if seen_types.contains(&(a, b)) { + // We've encountered a cycle. There's no point going any further -- the types are + // structurally the same. + return true; } - seen_types.mark_computing(a, b); + seen_types.insert((a, b)); let tcx = cx.tcx; - let result = if a == b || rustc_middle::ty::TyS::same_type(a, b) { + if a == b || rustc_middle::ty::TyS::same_type(a, b) { // All nominally-same types are structurally same, too. true } else { @@ -2333,11 +2293,9 @@ impl ClashingExternDeclarations { // uninitialised memory. _ => compare_layouts(a, b), } - }; - seen_types.mark_computed(a, b, result); - result + } } - let mut seen_types = SeenSet::new(); + let mut seen_types = FxHashSet::default(); structurally_same_type_impl(&mut seen_types, cx, a, b, ckind) } } From 7708abbbef679d208041bff57aa9ad50e9419895 Mon Sep 17 00:00:00 2001 From: jumbatm Date: Sun, 16 Aug 2020 17:14:09 +1000 Subject: [PATCH 08/10] Avoid double hashset lookup. Co-authored-by: Bastian Kauschke --- src/librustc_lint/builtin.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs index 781ebb6716758..8625dc096d500 100644 --- a/src/librustc_lint/builtin.rs +++ b/src/librustc_lint/builtin.rs @@ -2161,12 +2161,11 @@ impl ClashingExternDeclarations { ckind: CItemKind, ) -> bool { debug!("structurally_same_type_impl(cx, a = {:?}, b = {:?})", a, b); - if seen_types.contains(&(a, b)) { + if !seen_types.insert((a, b)) { // We've encountered a cycle. There's no point going any further -- the types are // structurally the same. return true; } - seen_types.insert((a, b)); let tcx = cx.tcx; if a == b || rustc_middle::ty::TyS::same_type(a, b) { // All nominally-same types are structurally same, too. From 1321a2dce32893588a0ec4ba586c3a04fb5e47cf Mon Sep 17 00:00:00 2001 From: jumbatm <30644300+jumbatm@users.noreply.github.com> Date: Sun, 16 Aug 2020 18:04:14 +1000 Subject: [PATCH 09/10] Also accept Refs for is_primitive_or_pointer --- src/librustc_lint/builtin.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs index 8625dc096d500..2439fc6660634 100644 --- a/src/librustc_lint/builtin.rs +++ b/src/librustc_lint/builtin.rs @@ -2184,8 +2184,9 @@ impl ClashingExternDeclarations { }; #[allow(rustc::usage_of_ty_tykind)] - let is_primitive_or_pointer = - |kind: &ty::TyKind<'_>| kind.is_primitive() || matches!(kind, RawPtr(..)); + let is_primitive_or_pointer = |kind: &ty::TyKind<'_>| { + kind.is_primitive() || matches!(kind, RawPtr(..) | Ref(..)) + }; match (a_kind, b_kind) { (Adt(a_def, a_substs), Adt(b_def, b_substs)) => { @@ -2274,8 +2275,8 @@ impl ClashingExternDeclarations { // These definitely should have been caught above. (Bool, Bool) | (Char, Char) | (Never, Never) | (Str, Str) => unreachable!(), - // An Adt and a primitive type. This can be FFI-safe is the ADT is an enum with a - // non-null field. + // An Adt and a primitive or pointer type. This can be FFI-safe if non-null + // enum layout optimisation is being applied. (Adt(..), other_kind) | (other_kind, Adt(..)) if is_primitive_or_pointer(other_kind) => { From bc15dd6dde58aaed724cf692c0635060a77fc99c Mon Sep 17 00:00:00 2001 From: jumbatm <30644300+jumbatm@users.noreply.github.com> Date: Tue, 18 Aug 2020 02:00:35 +1000 Subject: [PATCH 10/10] Wrap recursion in `ensure_sufficient_stack`. --- src/librustc_lint/builtin.rs | 179 ++++++++++++++++++----------------- 1 file changed, 91 insertions(+), 88 deletions(-) diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs index 2439fc6660634..147eb8f06c81c 100644 --- a/src/librustc_lint/builtin.rs +++ b/src/librustc_lint/builtin.rs @@ -29,6 +29,7 @@ use rustc_ast::tokenstream::{TokenStream, TokenTree}; use rustc_ast::visit::{FnCtxt, FnKind}; use rustc_ast_pretty::pprust::{self, expr_to_string}; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; +use rustc_data_structures::stack::ensure_sufficient_stack; use rustc_errors::{Applicability, DiagnosticBuilder, DiagnosticStyledString}; use rustc_feature::{deprecated_attributes, AttributeGate, AttributeTemplate, AttributeType}; use rustc_feature::{GateIssue, Stability}; @@ -2188,16 +2189,17 @@ impl ClashingExternDeclarations { kind.is_primitive() || matches!(kind, RawPtr(..) | Ref(..)) }; - match (a_kind, b_kind) { - (Adt(a_def, a_substs), Adt(b_def, b_substs)) => { - let a = a.subst(cx.tcx, a_substs); - let b = b.subst(cx.tcx, b_substs); - debug!("Comparing {:?} and {:?}", a, b); + ensure_sufficient_stack(|| { + match (a_kind, b_kind) { + (Adt(a_def, a_substs), Adt(b_def, b_substs)) => { + let a = a.subst(cx.tcx, a_substs); + let b = b.subst(cx.tcx, b_substs); + debug!("Comparing {:?} and {:?}", a, b); - // Grab a flattened representation of all fields. - let a_fields = a_def.variants.iter().flat_map(|v| v.fields.iter()); - let b_fields = b_def.variants.iter().flat_map(|v| v.fields.iter()); - compare_layouts(a, b) + // Grab a flattened representation of all fields. + let a_fields = a_def.variants.iter().flat_map(|v| v.fields.iter()); + let b_fields = b_def.variants.iter().flat_map(|v| v.fields.iter()); + compare_layouts(a, b) && a_fields.eq_by( b_fields, |&ty::FieldDef { did: a_did, .. }, @@ -2211,88 +2213,89 @@ impl ClashingExternDeclarations { ) }, ) - } - (Array(a_ty, a_const), Array(b_ty, b_const)) => { - // For arrays, we also check the constness of the type. - a_const.val == b_const.val - && structurally_same_type_impl(seen_types, cx, a_ty, b_ty, ckind) - } - (Slice(a_ty), Slice(b_ty)) => { - structurally_same_type_impl(seen_types, cx, a_ty, b_ty, ckind) - } - (RawPtr(a_tymut), RawPtr(b_tymut)) => { - a_tymut.mutbl == b_tymut.mutbl - && structurally_same_type_impl( - seen_types, - cx, - &a_tymut.ty, - &b_tymut.ty, - ckind, - ) - } - (Ref(_a_region, a_ty, a_mut), Ref(_b_region, b_ty, b_mut)) => { - // For structural sameness, we don't need the region to be same. - a_mut == b_mut - && structurally_same_type_impl(seen_types, cx, a_ty, b_ty, ckind) - } - (FnDef(..), FnDef(..)) => { - let a_poly_sig = a.fn_sig(tcx); - let b_poly_sig = b.fn_sig(tcx); - - // As we don't compare regions, skip_binder is fine. - let a_sig = a_poly_sig.skip_binder(); - let b_sig = b_poly_sig.skip_binder(); - - (a_sig.abi, a_sig.unsafety, a_sig.c_variadic) - == (b_sig.abi, b_sig.unsafety, b_sig.c_variadic) - && a_sig.inputs().iter().eq_by(b_sig.inputs().iter(), |a, b| { - structurally_same_type_impl(seen_types, cx, a, b, ckind) - }) - && structurally_same_type_impl( - seen_types, - cx, - a_sig.output(), - b_sig.output(), - ckind, - ) - } - (Tuple(a_substs), Tuple(b_substs)) => { - a_substs.types().eq_by(b_substs.types(), |a_ty, b_ty| { + } + (Array(a_ty, a_const), Array(b_ty, b_const)) => { + // For arrays, we also check the constness of the type. + a_const.val == b_const.val + && structurally_same_type_impl(seen_types, cx, a_ty, b_ty, ckind) + } + (Slice(a_ty), Slice(b_ty)) => { structurally_same_type_impl(seen_types, cx, a_ty, b_ty, ckind) - }) - } - // For these, it's not quite as easy to define structural-sameness quite so easily. - // For the purposes of this lint, take the conservative approach and mark them as - // not structurally same. - (Dynamic(..), Dynamic(..)) - | (Error(..), Error(..)) - | (Closure(..), Closure(..)) - | (Generator(..), Generator(..)) - | (GeneratorWitness(..), GeneratorWitness(..)) - | (Projection(..), Projection(..)) - | (Opaque(..), Opaque(..)) => false, - - // These definitely should have been caught above. - (Bool, Bool) | (Char, Char) | (Never, Never) | (Str, Str) => unreachable!(), - - // An Adt and a primitive or pointer type. This can be FFI-safe if non-null - // enum layout optimisation is being applied. - (Adt(..), other_kind) | (other_kind, Adt(..)) - if is_primitive_or_pointer(other_kind) => - { - let (primitive, adt) = - if is_primitive_or_pointer(&a.kind) { (a, b) } else { (b, a) }; - if let Some(ty) = crate::types::repr_nullable_ptr(cx, adt, ckind) { - ty == primitive - } else { - compare_layouts(a, b) } + (RawPtr(a_tymut), RawPtr(b_tymut)) => { + a_tymut.mutbl == b_tymut.mutbl + && structurally_same_type_impl( + seen_types, + cx, + &a_tymut.ty, + &b_tymut.ty, + ckind, + ) + } + (Ref(_a_region, a_ty, a_mut), Ref(_b_region, b_ty, b_mut)) => { + // For structural sameness, we don't need the region to be same. + a_mut == b_mut + && structurally_same_type_impl(seen_types, cx, a_ty, b_ty, ckind) + } + (FnDef(..), FnDef(..)) => { + let a_poly_sig = a.fn_sig(tcx); + let b_poly_sig = b.fn_sig(tcx); + + // As we don't compare regions, skip_binder is fine. + let a_sig = a_poly_sig.skip_binder(); + let b_sig = b_poly_sig.skip_binder(); + + (a_sig.abi, a_sig.unsafety, a_sig.c_variadic) + == (b_sig.abi, b_sig.unsafety, b_sig.c_variadic) + && a_sig.inputs().iter().eq_by(b_sig.inputs().iter(), |a, b| { + structurally_same_type_impl(seen_types, cx, a, b, ckind) + }) + && structurally_same_type_impl( + seen_types, + cx, + a_sig.output(), + b_sig.output(), + ckind, + ) + } + (Tuple(a_substs), Tuple(b_substs)) => { + a_substs.types().eq_by(b_substs.types(), |a_ty, b_ty| { + structurally_same_type_impl(seen_types, cx, a_ty, b_ty, ckind) + }) + } + // For these, it's not quite as easy to define structural-sameness quite so easily. + // For the purposes of this lint, take the conservative approach and mark them as + // not structurally same. + (Dynamic(..), Dynamic(..)) + | (Error(..), Error(..)) + | (Closure(..), Closure(..)) + | (Generator(..), Generator(..)) + | (GeneratorWitness(..), GeneratorWitness(..)) + | (Projection(..), Projection(..)) + | (Opaque(..), Opaque(..)) => false, + + // These definitely should have been caught above. + (Bool, Bool) | (Char, Char) | (Never, Never) | (Str, Str) => unreachable!(), + + // An Adt and a primitive or pointer type. This can be FFI-safe if non-null + // enum layout optimisation is being applied. + (Adt(..), other_kind) | (other_kind, Adt(..)) + if is_primitive_or_pointer(other_kind) => + { + let (primitive, adt) = + if is_primitive_or_pointer(&a.kind) { (a, b) } else { (b, a) }; + if let Some(ty) = crate::types::repr_nullable_ptr(cx, adt, ckind) { + ty == primitive + } else { + compare_layouts(a, b) + } + } + // Otherwise, just compare the layouts. This may fail to lint for some + // incompatible types, but at the very least, will stop reads into + // uninitialised memory. + _ => compare_layouts(a, b), } - // Otherwise, just compare the layouts. This may fail to lint for some - // incompatible types, but at the very least, will stop reads into - // uninitialised memory. - _ => compare_layouts(a, b), - } + }) } } let mut seen_types = FxHashSet::default();