Rollup merge of #130301 - RalfJung:clashing_extern_declarations, r=compiler-errors

some fixes for clashing_extern_declarations lint

There were two issues with the clashing_extern_declarations lint:
- It would accept non-`repr(C)` structs as compatible with each other by comparing their fields in declaration order, but the fields could have different memory order (and with `-Zrandomize-layout`, this can really happen).
- It would accept two types as compatible if `compare_layouts` returns `true`, but that function actually just compared the *ABI*, not the fully layout -- and all sized structs with more than 2 fields have the same ABI (`Abi::Aggregate`), so this missed a *lot* of cases.

We don't currently have a clear spec for what we *want* to consider "clashing" and what is fine, so I otherwise kept the original logic. I hope to have a t-lang discussion about this at some point. But meanwhile, these changes seem like clear bugfixes.

Author: matthiaskrgr

compiler/rustc_lint/src/foreign_modules.rs

@@ -3,8 +3,7 @@ use rustc_data_structures::unord::{UnordMap, UnordSet};
 use rustc_hir as hir;
 use rustc_hir::def::DefKind;
 use rustc_middle::query::Providers;
-use rustc_middle::ty::layout::LayoutError;
-use rustc_middle::ty::{self, Instance, Ty, TyCtxt};
+use rustc_middle::ty::{self, AdtDef, Instance, Ty, TyCtxt};
 use rustc_session::declare_lint;
 use rustc_span::{sym, Span, Symbol};
 use rustc_target::abi::FIRST_VARIANT;
@@ -212,7 +211,17 @@ fn structurally_same_type<'tcx>(
     ckind: types::CItemKind,
 ) -> bool {
     let mut seen_types = UnordSet::default();
-    structurally_same_type_impl(&mut seen_types, tcx, param_env, a, b, ckind)
+    let result = structurally_same_type_impl(&mut seen_types, tcx, param_env, a, b, ckind);
+    if cfg!(debug_assertions) && result {
+        // Sanity-check: must have same ABI, size and alignment.
+        // `extern` blocks cannot be generic, so we'll always get a layout here.
+        let a_layout = tcx.layout_of(param_env.and(a)).unwrap();
+        let b_layout = tcx.layout_of(param_env.and(b)).unwrap();
+        assert_eq!(a_layout.abi, b_layout.abi);
+        assert_eq!(a_layout.size, b_layout.size);
+        assert_eq!(a_layout.align, b_layout.align);
+    }
+    result
 }
 
 fn structurally_same_type_impl<'tcx>(
@@ -266,30 +275,21 @@ fn structurally_same_type_impl<'tcx>(
         // Do a full, depth-first comparison between the two.
         use rustc_type_ir::TyKind::*;
 
-        let compare_layouts = |a, b| -> Result<bool, &'tcx LayoutError<'tcx>> {
-            debug!("compare_layouts({:?}, {:?})", a, b);
-            let a_layout = &tcx.layout_of(param_env.and(a))?.layout.abi();
-            let b_layout = &tcx.layout_of(param_env.and(b))?.layout.abi();
-            debug!(
-                "comparing layouts: {:?} == {:?} = {}",
-                a_layout,
-                b_layout,
-                a_layout == b_layout
-            );
-            Ok(a_layout == b_layout)
-        };
-
         let is_primitive_or_pointer =
             |ty: Ty<'tcx>| ty.is_primitive() || matches!(ty.kind(), RawPtr(..) | Ref(..));
 
         ensure_sufficient_stack(|| {
             match (a.kind(), b.kind()) {
-                (Adt(a_def, _), Adt(b_def, _)) => {
-                    // We can immediately rule out these types as structurally same if
-                    // their layouts differ.
-                    match compare_layouts(a, b) {
-                        Ok(false) => return false,
-                        _ => (), // otherwise, continue onto the full, fields comparison
+                (&Adt(a_def, _), &Adt(b_def, _)) => {
+                    // Only `repr(C)` types can be compared structurally.
+                    if !(a_def.repr().c() && b_def.repr().c()) {
+                        return false;
+                    }
+                    // If the types differ in their packed-ness, align, or simd-ness they conflict.
+                    let repr_characteristica =
+                        |def: AdtDef<'tcx>| (def.repr().pack, def.repr().align, def.repr().simd());
+                    if repr_characteristica(a_def) != repr_characteristica(b_def) {
+                        return false;
                     }
 
                     // Grab a flattened representation of all fields.
@@ -311,9 +311,9 @@ fn structurally_same_type_impl<'tcx>(
                         },
                     )
                 }
-                (Array(a_ty, a_const), Array(b_ty, b_const)) => {
-                    // For arrays, we also check the constness of the type.
-                    a_const.kind() == b_const.kind()
+                (Array(a_ty, a_len), Array(b_ty, b_len)) => {
+                    // For arrays, we also check the length.
+                    a_len == b_len
                         && structurally_same_type_impl(
                             seen_types, tcx, param_env, *a_ty, *b_ty, ckind,
                         )
@@ -357,10 +357,9 @@ fn structurally_same_type_impl<'tcx>(
                             ckind,
                         )
                 }
-                (Tuple(a_args), Tuple(b_args)) => {
-                    a_args.iter().eq_by(b_args.iter(), |a_ty, b_ty| {
-                        structurally_same_type_impl(seen_types, tcx, param_env, a_ty, b_ty, ckind)
-                    })
+                (Tuple(..), Tuple(..)) => {
+                    // Tuples are not `repr(C)` so these cannot be compared structurally.
+                    false
                 }
                 // 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
@@ -380,24 +379,21 @@ fn structurally_same_type_impl<'tcx>(
                 // An Adt and a primitive or pointer type. This can be FFI-safe if non-null
                 // enum layout optimisation is being applied.
                 (Adt(..), _) if is_primitive_or_pointer(b) => {
-                    if let Some(ty) = types::repr_nullable_ptr(tcx, param_env, a, ckind) {
-                        ty == b
+                    if let Some(a_inner) = types::repr_nullable_ptr(tcx, param_env, a, ckind) {
+                        a_inner == b
                     } else {
-                        compare_layouts(a, b).unwrap_or(false)
+                        false
                     }
                 }
                 (_, Adt(..)) if is_primitive_or_pointer(a) => {
-                    if let Some(ty) = types::repr_nullable_ptr(tcx, param_env, b, ckind) {
-                        ty == a
+                    if let Some(b_inner) = types::repr_nullable_ptr(tcx, param_env, b, ckind) {
+                        b_inner == a
                     } else {
-                        compare_layouts(a, b).unwrap_or(false)
+                        false
                     }
                 }
 
-                // 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).unwrap_or(false),
+                _ => false,
             }
         })
     }

tests/ui/lint/clashing-extern-fn-recursion.rs

@@ -92,6 +92,7 @@ mod ref_recursion_once_removed {
             reffy: &'a Reffy2<'a>,
         }
 
+        #[repr(C)]
         struct Reffy2<'a> {
             reffy: &'a Reffy1<'a>,
         }
@@ -107,6 +108,7 @@ mod ref_recursion_once_removed {
             reffy: &'a Reffy2<'a>,
         }
 
+        #[repr(C)]
         struct Reffy2<'a> {
             reffy: &'a Reffy1<'a>,
         }

tests/ui/lint/clashing-extern-fn.rs

@@ -1,7 +1,6 @@
 //@ check-pass
 //@ aux-build:external_extern_fn.rs
 #![crate_type = "lib"]
-#![warn(clashing_extern_declarations)]
 
 mod redeclared_different_signature {
     mod a {
@@ -132,7 +131,7 @@ mod banana {
     mod three {
         // This _should_ trigger the lint, because repr(packed) should generate a struct that has a
         // different layout.
-        #[repr(packed)]
+        #[repr(C, packed)]
         struct Banana {
             weight: u32,
             length: u16,
@@ -143,6 +142,79 @@ mod banana {
             //~^ WARN `weigh_banana` redeclared with a different signature
         }
     }
+
+    mod four {
+        // This _should_ trigger the lint, because the type is not repr(C).
+        struct Banana {
+            weight: u32,
+            length: u16,
+        }
+        #[allow(improper_ctypes)]
+        extern "C" {
+            fn weigh_banana(count: *const Banana) -> u64;
+            //~^ WARN `weigh_banana` redeclared with a different signature
+        }
+    }
+}
+
+// 3-field structs can't be distinguished by ScalarPair, side-stepping some shortucts
+// the logic used to (incorrectly) take.
+mod banana3 {
+    mod one {
+        #[repr(C)]
+        struct Banana {
+            weight: u32,
+            length: u16,
+            color: u8,
+        }
+        extern "C" {
+            fn weigh_banana3(count: *const Banana) -> u64;
+        }
+    }
+
+    mod two {
+        #[repr(C)]
+        struct Banana {
+            weight: u32,
+            length: u16,
+            color: u8,
+        } // note: distinct type
+        // This should not trigger the lint because two::Banana is structurally equivalent to
+        // one::Banana.
+        extern "C" {
+            fn weigh_banana3(count: *const Banana) -> u64;
+        }
+    }
+
+    mod three {
+        // This _should_ trigger the lint, because repr(packed) should generate a struct that has a
+        // different layout.
+        #[repr(C, packed)]
+        struct Banana {
+            weight: u32,
+            length: u16,
+            color: u8,
+        }
+        #[allow(improper_ctypes)]
+        extern "C" {
+            fn weigh_banana3(count: *const Banana) -> u64;
+            //~^ WARN `weigh_banana3` redeclared with a different signature
+        }
+    }
+
+    mod four {
+        // This _should_ trigger the lint, because the type is not repr(C).
+        struct Banana {
+            weight: u32,
+            length: u16,
+            color: u8,
+        }
+        #[allow(improper_ctypes)]
+        extern "C" {
+            fn weigh_banana3(count: *const Banana) -> u64;
+            //~^ WARN `weigh_banana3` redeclared with a different signature
+        }
+    }
 }
 
 mod sameish_members {
@@ -223,27 +295,6 @@ mod transparent {
     }
 }
 
-#[allow(improper_ctypes)]
-mod zst {
-    mod transparent {
-        #[repr(transparent)]
-        struct TransparentZst(());
-        extern "C" {
-            fn zst() -> ();
-            fn transparent_zst() -> TransparentZst;
-        }
-    }
-
-    mod not_transparent {
-        struct NotTransparentZst(());
-        extern "C" {
-            // These shouldn't warn since all return types are zero sized
-            fn zst() -> NotTransparentZst;
-            fn transparent_zst() -> NotTransparentZst;
-        }
-    }
-}
-
 mod missing_return_type {
     mod a {
         extern "C" {
@@ -384,6 +435,7 @@ mod unknown_layout {
         extern "C" {
             pub fn generic(l: Link<u32>);
         }
+        #[repr(C)]
         pub struct Link<T> {
             pub item: T,
             pub next: *const Link<T>,
@@ -394,6 +446,7 @@ mod unknown_layout {
         extern "C" {
             pub fn generic(l: Link<u32>);
         }
+        #[repr(C)]
         pub struct Link<T> {
             pub item: T,
             pub next: *const Link<T>,