From e3f90bca50d4fddb52fae2ba5f7b5b19e12566e8 Mon Sep 17 00:00:00 2001 From: David Wood Date: Wed, 1 Mar 2023 15:52:29 +0000 Subject: [PATCH 1/6] lint/ctypes: ext. abi fn-ptr in internal abi fn Instead of skipping functions with internal ABIs, check that the signature doesn't contain any fn-ptr types with external ABIs that aren't FFI-safe. Signed-off-by: David Wood --- compiler/rustc_lint/src/types.rs | 75 +++++++++++++++---- tests/ui/abi/foreign/foreign-fn-with-byval.rs | 2 +- tests/ui/lint/lint-ctypes-94223.rs | 5 ++ tests/ui/lint/lint-ctypes-94223.stderr | 16 ++++ 4 files changed, 81 insertions(+), 17 deletions(-) create mode 100644 tests/ui/lint/lint-ctypes-94223.rs create mode 100644 tests/ui/lint/lint-ctypes-94223.stderr diff --git a/compiler/rustc_lint/src/types.rs b/compiler/rustc_lint/src/types.rs index 9a2d45ccd66..fefd431971e 100644 --- a/compiler/rustc_lint/src/types.rs +++ b/compiler/rustc_lint/src/types.rs @@ -1379,17 +1379,40 @@ fn check_type_for_ffi_and_report_errors( } } - fn check_foreign_fn(&mut self, def_id: LocalDefId, decl: &hir::FnDecl<'_>) { + /// Check if a function's argument types and result type are "ffi-safe". + /// + /// Argument types and the result type are checked for functions with external ABIs. + /// For functions with internal ABIs, argument types and the result type are walked to find + /// fn-ptr types that have external ABIs, as these still need checked. + fn check_maybe_foreign_fn(&mut self, abi: SpecAbi, def_id: LocalDefId, decl: &hir::FnDecl<'_>) { let sig = self.cx.tcx.fn_sig(def_id).subst_identity(); let sig = self.cx.tcx.erase_late_bound_regions(sig); + let is_internal_abi = self.is_internal_abi(abi); + let check_ty = |this: &mut ImproperCTypesVisitor<'a, 'tcx>, + span: Span, + ty: Ty<'tcx>, + is_return_type: bool| { + // If this function has an external ABI, then its arguments and return type should be + // checked.. + if !is_internal_abi { + this.check_type_for_ffi_and_report_errors(span, ty, false, is_return_type); + return; + } + + // ..but if this function has an internal ABI, then search the argument or return type + // for any fn-ptr types with external ABI, which should be checked.. + if let Some(fn_ptr_ty) = this.find_fn_ptr_ty_with_external_abi(ty) { + this.check_type_for_ffi_and_report_errors(span, fn_ptr_ty, false, is_return_type); + } + }; + for (input_ty, input_hir) in iter::zip(sig.inputs(), decl.inputs) { - self.check_type_for_ffi_and_report_errors(input_hir.span, *input_ty, false, false); + check_ty(self, input_hir.span, *input_ty, false); } if let hir::FnRetTy::Return(ref ret_hir) = decl.output { - let ret_ty = sig.output(); - self.check_type_for_ffi_and_report_errors(ret_hir.span, ret_ty, false, true); + check_ty(self, ret_hir.span, sig.output(), true); } } @@ -1404,6 +1427,30 @@ fn is_internal_abi(&self, abi: SpecAbi) -> bool { SpecAbi::Rust | SpecAbi::RustCall | SpecAbi::RustIntrinsic | SpecAbi::PlatformIntrinsic ) } + + /// Find any fn-ptr types with external ABIs in `ty`. + /// + /// For example, `Option` returns `extern "C" fn()` + fn find_fn_ptr_ty_with_external_abi(&self, ty: Ty<'tcx>) -> Option> { + struct FnPtrFinder<'parent, 'a, 'tcx>(&'parent ImproperCTypesVisitor<'a, 'tcx>); + impl<'vis, 'a, 'tcx> ty::visit::TypeVisitor> for FnPtrFinder<'vis, 'a, 'tcx> { + type BreakTy = Ty<'tcx>; + + fn visit_ty(&mut self, ty: Ty<'tcx>) -> ControlFlow { + if let ty::FnPtr(sig) = ty.kind() && !self.0.is_internal_abi(sig.abi()) { + ControlFlow::Break(ty) + } else { + ty.super_visit_with(self) + } + } + } + + self.cx + .tcx + .normalize_erasing_regions(self.cx.param_env, ty) + .visit_with(&mut FnPtrFinder(&*self)) + .break_value() + } } impl<'tcx> LateLintPass<'tcx> for ImproperCTypesDeclarations { @@ -1411,16 +1458,14 @@ fn check_foreign_item(&mut self, cx: &LateContext<'_>, it: &hir::ForeignItem<'_> let mut vis = ImproperCTypesVisitor { cx, mode: CItemKind::Declaration }; let abi = cx.tcx.hir().get_foreign_abi(it.hir_id()); - if !vis.is_internal_abi(abi) { - match it.kind { - hir::ForeignItemKind::Fn(ref decl, _, _) => { - vis.check_foreign_fn(it.owner_id.def_id, decl); - } - hir::ForeignItemKind::Static(ref ty, _) => { - vis.check_foreign_static(it.owner_id, ty.span); - } - hir::ForeignItemKind::Type => (), + match it.kind { + hir::ForeignItemKind::Fn(ref decl, _, _) => { + vis.check_maybe_foreign_fn(abi, it.owner_id.def_id, decl); } + hir::ForeignItemKind::Static(ref ty, _) if !vis.is_internal_abi(abi) => { + vis.check_foreign_static(it.owner_id, ty.span); + } + hir::ForeignItemKind::Static(..) | hir::ForeignItemKind::Type => (), } } } @@ -1444,9 +1489,7 @@ fn check_fn( }; let mut vis = ImproperCTypesVisitor { cx, mode: CItemKind::Definition }; - if !vis.is_internal_abi(abi) { - vis.check_foreign_fn(id, decl); - } + vis.check_maybe_foreign_fn(abi, id, decl); } } diff --git a/tests/ui/abi/foreign/foreign-fn-with-byval.rs b/tests/ui/abi/foreign/foreign-fn-with-byval.rs index f366b6ee1bd..e20ee0da45d 100644 --- a/tests/ui/abi/foreign/foreign-fn-with-byval.rs +++ b/tests/ui/abi/foreign/foreign-fn-with-byval.rs @@ -1,5 +1,5 @@ // run-pass -#![allow(improper_ctypes)] +#![allow(improper_ctypes, improper_ctypes_definitions)] // ignore-wasm32-bare no libc to test ffi with diff --git a/tests/ui/lint/lint-ctypes-94223.rs b/tests/ui/lint/lint-ctypes-94223.rs new file mode 100644 index 00000000000..ca7f3b73f66 --- /dev/null +++ b/tests/ui/lint/lint-ctypes-94223.rs @@ -0,0 +1,5 @@ +#![crate_type = "lib"] +#![deny(improper_ctypes_definitions)] + +pub fn bad(f: extern "C" fn([u8])) {} +//~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe diff --git a/tests/ui/lint/lint-ctypes-94223.stderr b/tests/ui/lint/lint-ctypes-94223.stderr new file mode 100644 index 00000000000..f8afed34ea4 --- /dev/null +++ b/tests/ui/lint/lint-ctypes-94223.stderr @@ -0,0 +1,16 @@ +error: `extern` fn uses type `[u8]`, which is not FFI-safe + --> $DIR/lint-ctypes-94223.rs:4:15 + | +LL | pub fn bad(f: extern "C" fn([u8])) {} + | ^^^^^^^^^^^^^^^^^^^ not FFI-safe + | + = help: consider using a raw pointer instead + = note: slices have no C equivalent +note: the lint level is defined here + --> $DIR/lint-ctypes-94223.rs:2:9 + | +LL | #![deny(improper_ctypes_definitions)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to previous error + From eddfce53c10d281cde6d283f36a12c43606b3f8c Mon Sep 17 00:00:00 2001 From: David Wood Date: Wed, 1 Mar 2023 15:55:09 +0000 Subject: [PATCH 2/6] abi: avoid ice for non-ffi-safe fn ptrs Remove an `unwrap` that assumed FFI-safe types in foreign fn-ptr types. Signed-off-by: David Wood --- compiler/rustc_target/src/abi/call/x86_64.rs | 10 ++++++---- tests/ui/abi/issue-94223.rs | 8 ++++++++ 2 files changed, 14 insertions(+), 4 deletions(-) create mode 100644 tests/ui/abi/issue-94223.rs diff --git a/compiler/rustc_target/src/abi/call/x86_64.rs b/compiler/rustc_target/src/abi/call/x86_64.rs index 74ef53915c9..b1aefaf0507 100644 --- a/compiler/rustc_target/src/abi/call/x86_64.rs +++ b/compiler/rustc_target/src/abi/call/x86_64.rs @@ -153,9 +153,9 @@ fn reg_component(cls: &[Option], i: &mut usize, size: Size) -> Option], size: Size) -> CastTarget { +fn cast_target(cls: &[Option], size: Size) -> Option { let mut i = 0; - let lo = reg_component(cls, &mut i, size).unwrap(); + let lo = reg_component(cls, &mut i, size)?; let offset = Size::from_bytes(8) * (i as u64); let mut target = CastTarget::from(lo); if size > offset { @@ -164,7 +164,7 @@ fn cast_target(cls: &[Option], size: Size) -> CastTarget { } } assert_eq!(reg_component(cls, &mut i, Size::ZERO), None); - target + Some(target) } const MAX_INT_REGS: usize = 6; // RDI, RSI, RDX, RCX, R8, R9 @@ -227,7 +227,9 @@ pub fn compute_abi_info<'a, Ty, C>(cx: &C, fn_abi: &mut FnAbi<'a, Ty>) // split into sized chunks passed individually if arg.layout.is_aggregate() { let size = arg.layout.size; - arg.cast_to(cast_target(cls, size)) + if let Some(cast_target) = cast_target(cls, size) { + arg.cast_to(cast_target); + } } else { arg.extend_integer_width_to(32); } diff --git a/tests/ui/abi/issue-94223.rs b/tests/ui/abi/issue-94223.rs new file mode 100644 index 00000000000..79d6b94031b --- /dev/null +++ b/tests/ui/abi/issue-94223.rs @@ -0,0 +1,8 @@ +// check-pass +#![allow(improper_ctypes_definitions)] +#![crate_type = "lib"] + +// Check that computing the fn abi for `bad`, with a external ABI fn ptr that is not FFI-safe, does +// not ICE. + +pub fn bad(f: extern "C" fn([u8])) {} From c75b080d7d878740c49539413d1b000906d420d4 Mon Sep 17 00:00:00 2001 From: David Wood Date: Thu, 2 Mar 2023 11:23:44 +0000 Subject: [PATCH 3/6] lint/ctypes: multiple external fn-ptrs in ty Extend previous commit's support for checking for external fn-ptrs in internal fn types to report errors for multiple found fn-ptrs. Signed-off-by: David Wood --- compiler/rustc_lint/src/types.rs | 63 ++++++++++++++++++-------- tests/ui/lint/lint-ctypes-94223.rs | 4 ++ tests/ui/lint/lint-ctypes-94223.stderr | 20 +++++++- 3 files changed, 68 insertions(+), 19 deletions(-) diff --git a/compiler/rustc_lint/src/types.rs b/compiler/rustc_lint/src/types.rs index fefd431971e..eeeac96798d 100644 --- a/compiler/rustc_lint/src/types.rs +++ b/compiler/rustc_lint/src/types.rs @@ -1384,35 +1384,40 @@ fn check_type_for_ffi_and_report_errors( /// Argument types and the result type are checked for functions with external ABIs. /// For functions with internal ABIs, argument types and the result type are walked to find /// fn-ptr types that have external ABIs, as these still need checked. - fn check_maybe_foreign_fn(&mut self, abi: SpecAbi, def_id: LocalDefId, decl: &hir::FnDecl<'_>) { + fn check_maybe_foreign_fn( + &mut self, + abi: SpecAbi, + def_id: LocalDefId, + decl: &'tcx hir::FnDecl<'_>, + ) { let sig = self.cx.tcx.fn_sig(def_id).subst_identity(); let sig = self.cx.tcx.erase_late_bound_regions(sig); let is_internal_abi = self.is_internal_abi(abi); let check_ty = |this: &mut ImproperCTypesVisitor<'a, 'tcx>, - span: Span, + hir_ty: &'tcx hir::Ty<'_>, ty: Ty<'tcx>, is_return_type: bool| { // If this function has an external ABI, then its arguments and return type should be // checked.. if !is_internal_abi { - this.check_type_for_ffi_and_report_errors(span, ty, false, is_return_type); + this.check_type_for_ffi_and_report_errors(hir_ty.span, ty, false, is_return_type); return; } // ..but if this function has an internal ABI, then search the argument or return type // for any fn-ptr types with external ABI, which should be checked.. - if let Some(fn_ptr_ty) = this.find_fn_ptr_ty_with_external_abi(ty) { + for (fn_ptr_ty, span) in this.find_fn_ptr_ty_with_external_abi(hir_ty, ty) { this.check_type_for_ffi_and_report_errors(span, fn_ptr_ty, false, is_return_type); } }; for (input_ty, input_hir) in iter::zip(sig.inputs(), decl.inputs) { - check_ty(self, input_hir.span, *input_ty, false); + check_ty(self, input_hir, *input_ty, false); } if let hir::FnRetTy::Return(ref ret_hir) = decl.output { - check_ty(self, ret_hir.span, sig.output(), true); + check_ty(self, ret_hir, sig.output(), true); } } @@ -1431,30 +1436,52 @@ fn is_internal_abi(&self, abi: SpecAbi) -> bool { /// Find any fn-ptr types with external ABIs in `ty`. /// /// For example, `Option` returns `extern "C" fn()` - fn find_fn_ptr_ty_with_external_abi(&self, ty: Ty<'tcx>) -> Option> { - struct FnPtrFinder<'parent, 'a, 'tcx>(&'parent ImproperCTypesVisitor<'a, 'tcx>); + fn find_fn_ptr_ty_with_external_abi( + &self, + hir_ty: &hir::Ty<'tcx>, + ty: Ty<'tcx>, + ) -> Vec<(Ty<'tcx>, Span)> { + struct FnPtrFinder<'parent, 'a, 'tcx> { + visitor: &'parent ImproperCTypesVisitor<'a, 'tcx>, + spans: Vec, + tys: Vec>, + } + + impl<'parent, 'a, 'tcx> hir::intravisit::Visitor<'_> for FnPtrFinder<'parent, 'a, 'tcx> { + fn visit_ty(&mut self, ty: &'_ hir::Ty<'_>) { + debug!(?ty); + if let hir::TyKind::BareFn(hir::BareFnTy { abi, .. }) = ty.kind + && !self.visitor.is_internal_abi(*abi) + { + self.spans.push(ty.span); + } + + hir::intravisit::walk_ty(self, ty) + } + } + impl<'vis, 'a, 'tcx> ty::visit::TypeVisitor> for FnPtrFinder<'vis, 'a, 'tcx> { type BreakTy = Ty<'tcx>; fn visit_ty(&mut self, ty: Ty<'tcx>) -> ControlFlow { - if let ty::FnPtr(sig) = ty.kind() && !self.0.is_internal_abi(sig.abi()) { - ControlFlow::Break(ty) - } else { - ty.super_visit_with(self) + if let ty::FnPtr(sig) = ty.kind() && !self.visitor.is_internal_abi(sig.abi()) { + self.tys.push(ty); } + + ty.super_visit_with(self) } } - self.cx - .tcx - .normalize_erasing_regions(self.cx.param_env, ty) - .visit_with(&mut FnPtrFinder(&*self)) - .break_value() + let mut visitor = FnPtrFinder { visitor: &*self, spans: Vec::new(), tys: Vec::new() }; + self.cx.tcx.normalize_erasing_regions(self.cx.param_env, ty).visit_with(&mut visitor); + hir::intravisit::Visitor::visit_ty(&mut visitor, hir_ty); + + iter::zip(visitor.tys.drain(..), visitor.spans.drain(..)).collect() } } impl<'tcx> LateLintPass<'tcx> for ImproperCTypesDeclarations { - fn check_foreign_item(&mut self, cx: &LateContext<'_>, it: &hir::ForeignItem<'_>) { + fn check_foreign_item(&mut self, cx: &LateContext<'tcx>, it: &hir::ForeignItem<'tcx>) { let mut vis = ImproperCTypesVisitor { cx, mode: CItemKind::Declaration }; let abi = cx.tcx.hir().get_foreign_abi(it.hir_id()); diff --git a/tests/ui/lint/lint-ctypes-94223.rs b/tests/ui/lint/lint-ctypes-94223.rs index ca7f3b73f66..98ccbd23a9e 100644 --- a/tests/ui/lint/lint-ctypes-94223.rs +++ b/tests/ui/lint/lint-ctypes-94223.rs @@ -3,3 +3,7 @@ pub fn bad(f: extern "C" fn([u8])) {} //~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe + +pub fn bad_twice(f: Result) {} +//~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe +//~^^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe diff --git a/tests/ui/lint/lint-ctypes-94223.stderr b/tests/ui/lint/lint-ctypes-94223.stderr index f8afed34ea4..e05d6197cb4 100644 --- a/tests/ui/lint/lint-ctypes-94223.stderr +++ b/tests/ui/lint/lint-ctypes-94223.stderr @@ -12,5 +12,23 @@ note: the lint level is defined here LL | #![deny(improper_ctypes_definitions)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to previous error +error: `extern` fn uses type `[u8]`, which is not FFI-safe + --> $DIR/lint-ctypes-94223.rs:7:28 + | +LL | pub fn bad_twice(f: Result) {} + | ^^^^^^^^^^^^^^^^^^^ not FFI-safe + | + = help: consider using a raw pointer instead + = note: slices have no C equivalent + +error: `extern` fn uses type `[u8]`, which is not FFI-safe + --> $DIR/lint-ctypes-94223.rs:7:49 + | +LL | pub fn bad_twice(f: Result) {} + | ^^^^^^^^^^^^^^^^^^^ not FFI-safe + | + = help: consider using a raw pointer instead + = note: slices have no C equivalent + +error: aborting due to 3 previous errors From 9137fea30dedd23fd78479732056fe2de29efeb8 Mon Sep 17 00:00:00 2001 From: David Wood Date: Thu, 2 Mar 2023 11:54:37 +0000 Subject: [PATCH 4/6] lint/ctypes: check other types for ext. fn-ptr ty Extend previous checks for external ABI fn-ptrs to use in internal statics, constants, type aliases and algebraic data types. Signed-off-by: David Wood --- compiler/rustc_lint/src/types.rs | 64 +++++++++++++++++- tests/ui/hashmap/hashmap-memory.rs | 1 + tests/ui/lint/lint-ctypes-94223.rs | 33 +++++++++ tests/ui/lint/lint-ctypes-94223.stderr | 94 +++++++++++++++++++++++++- 4 files changed, 190 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_lint/src/types.rs b/compiler/rustc_lint/src/types.rs index eeeac96798d..2f22dbbeb14 100644 --- a/compiler/rustc_lint/src/types.rs +++ b/compiler/rustc_lint/src/types.rs @@ -1473,7 +1473,11 @@ fn visit_ty(&mut self, ty: Ty<'tcx>) -> ControlFlow { } let mut visitor = FnPtrFinder { visitor: &*self, spans: Vec::new(), tys: Vec::new() }; - self.cx.tcx.normalize_erasing_regions(self.cx.param_env, ty).visit_with(&mut visitor); + self.cx + .tcx + .try_normalize_erasing_regions(self.cx.param_env, ty) + .unwrap_or(ty) + .visit_with(&mut visitor); hir::intravisit::Visitor::visit_ty(&mut visitor, hir_ty); iter::zip(visitor.tys.drain(..), visitor.spans.drain(..)).collect() @@ -1497,7 +1501,65 @@ fn check_foreign_item(&mut self, cx: &LateContext<'tcx>, it: &hir::ForeignItem<' } } +impl ImproperCTypesDefinitions { + fn check_ty_maybe_containing_foreign_fnptr<'tcx>( + &mut self, + cx: &LateContext<'tcx>, + hir_ty: &'tcx hir::Ty<'_>, + ty: Ty<'tcx>, + ) { + let mut vis = ImproperCTypesVisitor { cx, mode: CItemKind::Definition }; + for (fn_ptr_ty, span) in vis.find_fn_ptr_ty_with_external_abi(hir_ty, ty) { + vis.check_type_for_ffi_and_report_errors(span, fn_ptr_ty, true, false); + } + } +} + +/// `ImproperCTypesDefinitions` checks items outside of foreign items (e.g. stuff that isn't in +/// `extern "C" { }` blocks): +/// +/// - `extern "" fn` definitions are checked in the same way as the +/// `ImproperCtypesDeclarations` visitor checks functions if `` is external (e.g. "C"). +/// - All other items which contain types (e.g. other functions, struct definitions, etc) are +/// checked for extern fn-ptrs with external ABIs. impl<'tcx> LateLintPass<'tcx> for ImproperCTypesDefinitions { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'tcx>) { + match item.kind { + hir::ItemKind::Static(ty, ..) + | hir::ItemKind::Const(ty, ..) + | hir::ItemKind::TyAlias(ty, ..) => { + self.check_ty_maybe_containing_foreign_fnptr( + cx, + ty, + cx.tcx.type_of(item.owner_id).subst_identity(), + ); + } + // See `check_fn`.. + hir::ItemKind::Fn(..) => {} + // See `check_field_def`.. + hir::ItemKind::Union(..) | hir::ItemKind::Struct(..) | hir::ItemKind::Enum(..) => {} + // Doesn't define something that can contain a external type to be checked. + hir::ItemKind::Impl(..) + | hir::ItemKind::TraitAlias(..) + | hir::ItemKind::Trait(..) + | hir::ItemKind::OpaqueTy(..) + | hir::ItemKind::GlobalAsm(..) + | hir::ItemKind::ForeignMod { .. } + | hir::ItemKind::Mod(..) + | hir::ItemKind::Macro(..) + | hir::ItemKind::Use(..) + | hir::ItemKind::ExternCrate(..) => {} + } + } + + fn check_field_def(&mut self, cx: &LateContext<'tcx>, field: &'tcx hir::FieldDef<'tcx>) { + self.check_ty_maybe_containing_foreign_fnptr( + cx, + field.ty, + cx.tcx.type_of(field.def_id).subst_identity(), + ); + } + fn check_fn( &mut self, cx: &LateContext<'tcx>, diff --git a/tests/ui/hashmap/hashmap-memory.rs b/tests/ui/hashmap/hashmap-memory.rs index 87f8b6ad573..bd364b349e2 100644 --- a/tests/ui/hashmap/hashmap-memory.rs +++ b/tests/ui/hashmap/hashmap-memory.rs @@ -1,5 +1,6 @@ // run-pass +#![allow(improper_ctypes_definitions)] #![allow(non_camel_case_types)] #![allow(dead_code)] #![allow(unused_mut)] diff --git a/tests/ui/lint/lint-ctypes-94223.rs b/tests/ui/lint/lint-ctypes-94223.rs index 98ccbd23a9e..70dd2a71f26 100644 --- a/tests/ui/lint/lint-ctypes-94223.rs +++ b/tests/ui/lint/lint-ctypes-94223.rs @@ -7,3 +7,36 @@ pub fn bad(f: extern "C" fn([u8])) {} pub fn bad_twice(f: Result) {} //~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe //~^^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe + +struct BadStruct(extern "C" fn([u8])); +//~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe + +enum BadEnum { + A(extern "C" fn([u8])), + //~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe +} + +enum BadUnion { + A(extern "C" fn([u8])), + //~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe +} + +type Foo = extern "C" fn([u8]); +//~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe + +pub struct FfiUnsafe; + +#[allow(improper_ctypes_definitions)] +extern "C" fn f(_: FfiUnsafe) { + unimplemented!() +} + +pub static BAD: extern "C" fn(FfiUnsafe) = f; +//~^ ERROR `extern` fn uses type `FfiUnsafe`, which is not FFI-safe + +pub static BAD_TWICE: Result = Ok(f); +//~^ ERROR `extern` fn uses type `FfiUnsafe`, which is not FFI-safe +//~^^ ERROR `extern` fn uses type `FfiUnsafe`, which is not FFI-safe + +pub const BAD_CONST: extern "C" fn(FfiUnsafe) = f; +//~^ ERROR `extern` fn uses type `FfiUnsafe`, which is not FFI-safe diff --git a/tests/ui/lint/lint-ctypes-94223.stderr b/tests/ui/lint/lint-ctypes-94223.stderr index e05d6197cb4..49e64ed5140 100644 --- a/tests/ui/lint/lint-ctypes-94223.stderr +++ b/tests/ui/lint/lint-ctypes-94223.stderr @@ -30,5 +30,97 @@ LL | pub fn bad_twice(f: Result) {} = help: consider using a raw pointer instead = note: slices have no C equivalent -error: aborting due to 3 previous errors +error: `extern` fn uses type `[u8]`, which is not FFI-safe + --> $DIR/lint-ctypes-94223.rs:11:18 + | +LL | struct BadStruct(extern "C" fn([u8])); + | ^^^^^^^^^^^^^^^^^^^ not FFI-safe + | + = help: consider using a raw pointer instead + = note: slices have no C equivalent + +error: `extern` fn uses type `[u8]`, which is not FFI-safe + --> $DIR/lint-ctypes-94223.rs:15:7 + | +LL | A(extern "C" fn([u8])), + | ^^^^^^^^^^^^^^^^^^^ not FFI-safe + | + = help: consider using a raw pointer instead + = note: slices have no C equivalent + +error: `extern` fn uses type `[u8]`, which is not FFI-safe + --> $DIR/lint-ctypes-94223.rs:20:7 + | +LL | A(extern "C" fn([u8])), + | ^^^^^^^^^^^^^^^^^^^ not FFI-safe + | + = help: consider using a raw pointer instead + = note: slices have no C equivalent + +error: `extern` fn uses type `[u8]`, which is not FFI-safe + --> $DIR/lint-ctypes-94223.rs:24:12 + | +LL | type Foo = extern "C" fn([u8]); + | ^^^^^^^^^^^^^^^^^^^ not FFI-safe + | + = help: consider using a raw pointer instead + = note: slices have no C equivalent + +error: `extern` fn uses type `FfiUnsafe`, which is not FFI-safe + --> $DIR/lint-ctypes-94223.rs:34:17 + | +LL | pub static BAD: extern "C" fn(FfiUnsafe) = f; + | ^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe + | + = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct + = note: this struct has unspecified layout +note: the type is defined here + --> $DIR/lint-ctypes-94223.rs:27:1 + | +LL | pub struct FfiUnsafe; + | ^^^^^^^^^^^^^^^^^^^^ + +error: `extern` fn uses type `FfiUnsafe`, which is not FFI-safe + --> $DIR/lint-ctypes-94223.rs:37:30 + | +LL | pub static BAD_TWICE: Result = Ok(f); + | ^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe + | + = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct + = note: this struct has unspecified layout +note: the type is defined here + --> $DIR/lint-ctypes-94223.rs:27:1 + | +LL | pub struct FfiUnsafe; + | ^^^^^^^^^^^^^^^^^^^^ + +error: `extern` fn uses type `FfiUnsafe`, which is not FFI-safe + --> $DIR/lint-ctypes-94223.rs:37:56 + | +LL | pub static BAD_TWICE: Result = Ok(f); + | ^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe + | + = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct + = note: this struct has unspecified layout +note: the type is defined here + --> $DIR/lint-ctypes-94223.rs:27:1 + | +LL | pub struct FfiUnsafe; + | ^^^^^^^^^^^^^^^^^^^^ + +error: `extern` fn uses type `FfiUnsafe`, which is not FFI-safe + --> $DIR/lint-ctypes-94223.rs:41:22 + | +LL | pub const BAD_CONST: extern "C" fn(FfiUnsafe) = f; + | ^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe + | + = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct + = note: this struct has unspecified layout +note: the type is defined here + --> $DIR/lint-ctypes-94223.rs:27:1 + | +LL | pub struct FfiUnsafe; + | ^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 11 previous errors From f1e287948b57a11114b76bc506faf25675e9d3ec Mon Sep 17 00:00:00 2001 From: David Wood Date: Mon, 3 Jul 2023 14:12:20 +0100 Subject: [PATCH 5/6] lint: stop normalizing types to avoid recur limits This was causing compilation failures in the performance benchmarking as diesel hit recursion limits. Signed-off-by: David Wood --- compiler/rustc_lint/src/types.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/compiler/rustc_lint/src/types.rs b/compiler/rustc_lint/src/types.rs index 2f22dbbeb14..31d26c4b30f 100644 --- a/compiler/rustc_lint/src/types.rs +++ b/compiler/rustc_lint/src/types.rs @@ -1473,11 +1473,7 @@ fn visit_ty(&mut self, ty: Ty<'tcx>) -> ControlFlow { } let mut visitor = FnPtrFinder { visitor: &*self, spans: Vec::new(), tys: Vec::new() }; - self.cx - .tcx - .try_normalize_erasing_regions(self.cx.param_env, ty) - .unwrap_or(ty) - .visit_with(&mut visitor); + ty.visit_with(&mut visitor); hir::intravisit::Visitor::visit_ty(&mut visitor, hir_ty); iter::zip(visitor.tys.drain(..), visitor.spans.drain(..)).collect() From 2227422920e544c1e73050f6ef1f687349e13a7d Mon Sep 17 00:00:00 2001 From: David Wood Date: Mon, 3 Jul 2023 14:29:04 +0100 Subject: [PATCH 6/6] lint: refactor to make logic a bit cleaner Signed-off-by: David Wood --- compiler/rustc_lint/src/types.rs | 64 ++++++++++++++++---------------- 1 file changed, 31 insertions(+), 33 deletions(-) diff --git a/compiler/rustc_lint/src/types.rs b/compiler/rustc_lint/src/types.rs index 31d26c4b30f..0ae0306b5ac 100644 --- a/compiler/rustc_lint/src/types.rs +++ b/compiler/rustc_lint/src/types.rs @@ -1381,43 +1381,36 @@ fn check_type_for_ffi_and_report_errors( /// Check if a function's argument types and result type are "ffi-safe". /// - /// Argument types and the result type are checked for functions with external ABIs. - /// For functions with internal ABIs, argument types and the result type are walked to find - /// fn-ptr types that have external ABIs, as these still need checked. - fn check_maybe_foreign_fn( - &mut self, - abi: SpecAbi, - def_id: LocalDefId, - decl: &'tcx hir::FnDecl<'_>, - ) { + /// For a external ABI function, argument types and the result type are walked to find fn-ptr + /// types that have external ABIs, as these still need checked. + fn check_fn(&mut self, def_id: LocalDefId, decl: &'tcx hir::FnDecl<'_>) { let sig = self.cx.tcx.fn_sig(def_id).subst_identity(); let sig = self.cx.tcx.erase_late_bound_regions(sig); - let is_internal_abi = self.is_internal_abi(abi); - let check_ty = |this: &mut ImproperCTypesVisitor<'a, 'tcx>, - hir_ty: &'tcx hir::Ty<'_>, - ty: Ty<'tcx>, - is_return_type: bool| { - // If this function has an external ABI, then its arguments and return type should be - // checked.. - if !is_internal_abi { - this.check_type_for_ffi_and_report_errors(hir_ty.span, ty, false, is_return_type); - return; - } - - // ..but if this function has an internal ABI, then search the argument or return type - // for any fn-ptr types with external ABI, which should be checked.. - for (fn_ptr_ty, span) in this.find_fn_ptr_ty_with_external_abi(hir_ty, ty) { - this.check_type_for_ffi_and_report_errors(span, fn_ptr_ty, false, is_return_type); - } - }; - for (input_ty, input_hir) in iter::zip(sig.inputs(), decl.inputs) { - check_ty(self, input_hir, *input_ty, false); + for (fn_ptr_ty, span) in self.find_fn_ptr_ty_with_external_abi(input_hir, *input_ty) { + self.check_type_for_ffi_and_report_errors(span, fn_ptr_ty, false, false); + } } if let hir::FnRetTy::Return(ref ret_hir) = decl.output { - check_ty(self, ret_hir, sig.output(), true); + for (fn_ptr_ty, span) in self.find_fn_ptr_ty_with_external_abi(ret_hir, sig.output()) { + self.check_type_for_ffi_and_report_errors(span, fn_ptr_ty, false, true); + } + } + } + + /// Check if a function's argument types and result type are "ffi-safe". + fn check_foreign_fn(&mut self, def_id: LocalDefId, decl: &'tcx hir::FnDecl<'_>) { + let sig = self.cx.tcx.fn_sig(def_id).subst_identity(); + let sig = self.cx.tcx.erase_late_bound_regions(sig); + + for (input_ty, input_hir) in iter::zip(sig.inputs(), decl.inputs) { + self.check_type_for_ffi_and_report_errors(input_hir.span, *input_ty, false, false); + } + + if let hir::FnRetTy::Return(ref ret_hir) = decl.output { + self.check_type_for_ffi_and_report_errors(ret_hir.span, sig.output(), false, true); } } @@ -1486,12 +1479,13 @@ fn check_foreign_item(&mut self, cx: &LateContext<'tcx>, it: &hir::ForeignItem<' let abi = cx.tcx.hir().get_foreign_abi(it.hir_id()); match it.kind { - hir::ForeignItemKind::Fn(ref decl, _, _) => { - vis.check_maybe_foreign_fn(abi, it.owner_id.def_id, decl); + hir::ForeignItemKind::Fn(ref decl, _, _) if !vis.is_internal_abi(abi) => { + vis.check_foreign_fn(it.owner_id.def_id, decl); } hir::ForeignItemKind::Static(ref ty, _) if !vis.is_internal_abi(abi) => { vis.check_foreign_static(it.owner_id, ty.span); } + hir::ForeignItemKind::Fn(ref decl, _, _) => vis.check_fn(it.owner_id.def_id, decl), hir::ForeignItemKind::Static(..) | hir::ForeignItemKind::Type => (), } } @@ -1574,7 +1568,11 @@ fn check_fn( }; let mut vis = ImproperCTypesVisitor { cx, mode: CItemKind::Definition }; - vis.check_maybe_foreign_fn(abi, id, decl); + if vis.is_internal_abi(abi) { + vis.check_fn(id, decl); + } else { + vis.check_foreign_fn(id, decl); + } } }