From f362a59c3ed6241e515e2516a048945a879bbfe3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 13 Sep 2024 11:13:59 +0200 Subject: [PATCH] some fixes for clashing_extern_declarations lint --- compiler/rustc_lint/src/foreign_modules.rs | 74 +++++++------- tests/ui/lint/clashing-extern-fn-recursion.rs | 2 + tests/ui/lint/clashing-extern-fn.rs | 99 ++++++++++++++----- tests/ui/lint/clashing-extern-fn.stderr | 86 +++++++++++----- 4 files changed, 172 insertions(+), 89 deletions(-) diff --git a/compiler/rustc_lint/src/foreign_modules.rs b/compiler/rustc_lint/src/foreign_modules.rs index a60fc0ffbbb..33b8b7c544b 100644 --- a/compiler/rustc_lint/src/foreign_modules.rs +++ b/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> { - 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, } }) } diff --git a/tests/ui/lint/clashing-extern-fn-recursion.rs b/tests/ui/lint/clashing-extern-fn-recursion.rs index 40bef400594..ab311d398f9 100644 --- a/tests/ui/lint/clashing-extern-fn-recursion.rs +++ b/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>, } diff --git a/tests/ui/lint/clashing-extern-fn.rs b/tests/ui/lint/clashing-extern-fn.rs index 728dfabb393..a12fe81eecd 100644 --- a/tests/ui/lint/clashing-extern-fn.rs +++ b/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); } + #[repr(C)] pub struct Link { pub item: T, pub next: *const Link, @@ -394,6 +446,7 @@ mod unknown_layout { extern "C" { pub fn generic(l: Link); } + #[repr(C)] pub struct Link { pub item: T, pub next: *const Link, diff --git a/tests/ui/lint/clashing-extern-fn.stderr b/tests/ui/lint/clashing-extern-fn.stderr index f75ff6d05a1..b30dd476a1d 100644 --- a/tests/ui/lint/clashing-extern-fn.stderr +++ b/tests/ui/lint/clashing-extern-fn.stderr @@ -1,5 +1,5 @@ warning: `extern` block uses type `Option`, which is not FFI-safe - --> $DIR/clashing-extern-fn.rs:429:55 + --> $DIR/clashing-extern-fn.rs:482:55 | LL | fn hidden_niche_transparent_no_niche() -> Option; | ^^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe @@ -9,7 +9,7 @@ LL | fn hidden_niche_transparent_no_niche() -> Option>>`, which is not FFI-safe - --> $DIR/clashing-extern-fn.rs:433:46 + --> $DIR/clashing-extern-fn.rs:486:46 | LL | fn hidden_niche_unsafe_cell() -> Option>>; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe @@ -18,7 +18,7 @@ LL | fn hidden_niche_unsafe_cell() -> Option $DIR/clashing-extern-fn.rs:14:13 + --> $DIR/clashing-extern-fn.rs:13:13 | LL | fn clash(x: u8); | ---------------- `clash` previously declared here @@ -28,14 +28,10 @@ LL | fn clash(x: u64); | = note: expected `unsafe extern "C" fn(u8)` found `unsafe extern "C" fn(u64)` -note: the lint level is defined here - --> $DIR/clashing-extern-fn.rs:4:9 - | -LL | #![warn(clashing_extern_declarations)] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: `#[warn(clashing_extern_declarations)]` on by default warning: `extern_link_name` redeclared with a different signature - --> $DIR/clashing-extern-fn.rs:52:9 + --> $DIR/clashing-extern-fn.rs:51:9 | LL | #[link_name = "extern_link_name"] | --------------------------------- `extern_link_name` previously declared here @@ -47,7 +43,7 @@ LL | fn extern_link_name(x: u32); found `unsafe extern "C" fn(u32)` warning: `some_other_extern_link_name` redeclares `some_other_new_name` with a different signature - --> $DIR/clashing-extern-fn.rs:55:9 + --> $DIR/clashing-extern-fn.rs:54:9 | LL | fn some_other_new_name(x: i16); | ------------------------------- `some_other_new_name` previously declared here @@ -59,7 +55,7 @@ LL | #[link_name = "some_other_new_name"] found `unsafe extern "C" fn(u32)` warning: `other_both_names_different` redeclares `link_name_same` with a different signature - --> $DIR/clashing-extern-fn.rs:59:9 + --> $DIR/clashing-extern-fn.rs:58:9 | LL | #[link_name = "link_name_same"] | ------------------------------- `link_name_same` previously declared here @@ -71,7 +67,7 @@ LL | #[link_name = "link_name_same"] found `unsafe extern "C" fn(u32)` warning: `different_mod` redeclared with a different signature - --> $DIR/clashing-extern-fn.rs:72:9 + --> $DIR/clashing-extern-fn.rs:71:9 | LL | fn different_mod(x: u8); | ------------------------ `different_mod` previously declared here @@ -83,7 +79,7 @@ LL | fn different_mod(x: u64); found `unsafe extern "C" fn(u64)` warning: `variadic_decl` redeclared with a different signature - --> $DIR/clashing-extern-fn.rs:82:9 + --> $DIR/clashing-extern-fn.rs:81:9 | LL | fn variadic_decl(x: u8, ...); | ----------------------------- `variadic_decl` previously declared here @@ -95,7 +91,7 @@ LL | fn variadic_decl(x: u8); found `unsafe extern "C" fn(u8)` warning: `weigh_banana` redeclared with a different signature - --> $DIR/clashing-extern-fn.rs:142:13 + --> $DIR/clashing-extern-fn.rs:141:13 | LL | fn weigh_banana(count: *const Banana) -> u64; | --------------------------------------------- `weigh_banana` previously declared here @@ -103,11 +99,47 @@ LL | fn weigh_banana(count: *const Banana) -> u64; LL | fn weigh_banana(count: *const Banana) -> u64; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration | - = note: expected `unsafe extern "C" fn(*const one::Banana) -> u64` - found `unsafe extern "C" fn(*const three::Banana) -> u64` + = note: expected `unsafe extern "C" fn(*const banana::one::Banana) -> u64` + found `unsafe extern "C" fn(*const banana::three::Banana) -> u64` + +warning: `weigh_banana` redeclared with a different signature + --> $DIR/clashing-extern-fn.rs:154:13 + | +LL | fn weigh_banana(count: *const Banana) -> u64; + | --------------------------------------------- `weigh_banana` previously declared here +... +LL | fn weigh_banana(count: *const Banana) -> u64; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration + | + = note: expected `unsafe extern "C" fn(*const banana::one::Banana) -> u64` + found `unsafe extern "C" fn(*const banana::four::Banana) -> u64` + +warning: `weigh_banana3` redeclared with a different signature + --> $DIR/clashing-extern-fn.rs:200:13 + | +LL | fn weigh_banana3(count: *const Banana) -> u64; + | ---------------------------------------------- `weigh_banana3` previously declared here +... +LL | fn weigh_banana3(count: *const Banana) -> u64; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration + | + = note: expected `unsafe extern "C" fn(*const banana3::one::Banana) -> u64` + found `unsafe extern "C" fn(*const banana3::three::Banana) -> u64` + +warning: `weigh_banana3` redeclared with a different signature + --> $DIR/clashing-extern-fn.rs:214:13 + | +LL | fn weigh_banana3(count: *const Banana) -> u64; + | ---------------------------------------------- `weigh_banana3` previously declared here +... +LL | fn weigh_banana3(count: *const Banana) -> u64; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration + | + = note: expected `unsafe extern "C" fn(*const banana3::one::Banana) -> u64` + found `unsafe extern "C" fn(*const banana3::four::Banana) -> u64` warning: `draw_point` redeclared with a different signature - --> $DIR/clashing-extern-fn.rs:171:13 + --> $DIR/clashing-extern-fn.rs:243:13 | LL | fn draw_point(p: Point); | ------------------------ `draw_point` previously declared here @@ -119,7 +151,7 @@ LL | fn draw_point(p: Point); found `unsafe extern "C" fn(sameish_members::b::Point)` warning: `origin` redeclared with a different signature - --> $DIR/clashing-extern-fn.rs:197:13 + --> $DIR/clashing-extern-fn.rs:269:13 | LL | fn origin() -> Point3; | ---------------------- `origin` previously declared here @@ -131,7 +163,7 @@ LL | fn origin() -> Point3; found `unsafe extern "C" fn() -> same_sized_members_clash::b::Point3` warning: `transparent_incorrect` redeclared with a different signature - --> $DIR/clashing-extern-fn.rs:220:13 + --> $DIR/clashing-extern-fn.rs:292:13 | LL | fn transparent_incorrect() -> T; | -------------------------------- `transparent_incorrect` previously declared here @@ -143,7 +175,7 @@ LL | fn transparent_incorrect() -> isize; found `unsafe extern "C" fn() -> isize` warning: `missing_return_type` redeclared with a different signature - --> $DIR/clashing-extern-fn.rs:259:13 + --> $DIR/clashing-extern-fn.rs:310:13 | LL | fn missing_return_type() -> usize; | ---------------------------------- `missing_return_type` previously declared here @@ -155,7 +187,7 @@ LL | fn missing_return_type(); found `unsafe extern "C" fn()` warning: `non_zero_usize` redeclared with a different signature - --> $DIR/clashing-extern-fn.rs:277:13 + --> $DIR/clashing-extern-fn.rs:328:13 | LL | fn non_zero_usize() -> core::num::NonZero; | ------------------------------------------------- `non_zero_usize` previously declared here @@ -167,7 +199,7 @@ LL | fn non_zero_usize() -> usize; found `unsafe extern "C" fn() -> usize` warning: `non_null_ptr` redeclared with a different signature - --> $DIR/clashing-extern-fn.rs:279:13 + --> $DIR/clashing-extern-fn.rs:330:13 | LL | fn non_null_ptr() -> core::ptr::NonNull; | ----------------------------------------------- `non_null_ptr` previously declared here @@ -179,7 +211,7 @@ LL | fn non_null_ptr() -> *const usize; found `unsafe extern "C" fn() -> *const usize` warning: `option_non_zero_usize_incorrect` redeclared with a different signature - --> $DIR/clashing-extern-fn.rs:373:13 + --> $DIR/clashing-extern-fn.rs:424:13 | LL | fn option_non_zero_usize_incorrect() -> usize; | ---------------------------------------------- `option_non_zero_usize_incorrect` previously declared here @@ -191,7 +223,7 @@ LL | fn option_non_zero_usize_incorrect() -> isize; found `unsafe extern "C" fn() -> isize` warning: `option_non_null_ptr_incorrect` redeclared with a different signature - --> $DIR/clashing-extern-fn.rs:375:13 + --> $DIR/clashing-extern-fn.rs:426:13 | LL | fn option_non_null_ptr_incorrect() -> *const usize; | --------------------------------------------------- `option_non_null_ptr_incorrect` previously declared here @@ -203,7 +235,7 @@ LL | fn option_non_null_ptr_incorrect() -> *const isize; found `unsafe extern "C" fn() -> *const isize` warning: `hidden_niche_transparent_no_niche` redeclared with a different signature - --> $DIR/clashing-extern-fn.rs:429:13 + --> $DIR/clashing-extern-fn.rs:482:13 | LL | fn hidden_niche_transparent_no_niche() -> usize; | ------------------------------------------------ `hidden_niche_transparent_no_niche` previously declared here @@ -215,7 +247,7 @@ LL | fn hidden_niche_transparent_no_niche() -> Option Option` warning: `hidden_niche_unsafe_cell` redeclared with a different signature - --> $DIR/clashing-extern-fn.rs:433:13 + --> $DIR/clashing-extern-fn.rs:486:13 | LL | fn hidden_niche_unsafe_cell() -> usize; | --------------------------------------- `hidden_niche_unsafe_cell` previously declared here @@ -226,5 +258,5 @@ LL | fn hidden_niche_unsafe_cell() -> Option usize` found `unsafe extern "C" fn() -> Option>>` -warning: 19 warnings emitted +warning: 22 warnings emitted