Skip to content

some fixes for clashing_extern_declarations lint #130301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 13, 2024
Merged

some fixes for clashing_extern_declarations lint #130301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 35 additions & 39 deletions compiler/rustc_lint/src/foreign_modules.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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>(
Expand Down Expand Up @@ -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)
Copy link
Member Author

@RalfJung RalfJung Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up removing this logic entirely. It is never sound to do this, and none of the tests relied on this, so the motivation is unclear.

If this warns for too many real-world cases, people will tell us. :)

};

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.
Expand All @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the constness" 💀

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah...

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,
)
Expand Down Expand Up @@ -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
Expand All @@ -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,
}
})
}
Expand Down
2 changes: 2 additions & 0 deletions tests/ui/lint/clashing-extern-fn-recursion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ mod ref_recursion_once_removed {
reffy: &'a Reffy2<'a>,
}

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

#[repr(C)]
struct Reffy2<'a> {
reffy: &'a Reffy1<'a>,
}
Expand Down
99 changes: 76 additions & 23 deletions tests/ui/lint/clashing-extern-fn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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,
Expand All @@ -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 {
Expand Down Expand Up @@ -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" {
Expand Down Expand Up @@ -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>,
Expand All @@ -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>,
Expand Down
Loading
Loading