Skip to content

Commit 0ab38e9

Browse files
committed
Auto merge of #108611 - davidtwco:issue-94223-external-abi-fn-ptr-in-internal-abi-fn, r=jackh726
lint/ctypes: ext. abi fn-ptr in internal abi fn Fixes #94223. - In the improper ctypes lint, 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. - When computing the ABI for fn-ptr types, remove an `unwrap` that assumed FFI-safe types in foreign fn-ptr types. - I'm not certain that this is the correct approach.
2 parents 8931edf + 2227422 commit 0ab38e9

File tree

7 files changed

+324
-19
lines changed

7 files changed

+324
-19
lines changed

compiler/rustc_lint/src/types.rs

+140-14
Original file line numberDiff line numberDiff line change
@@ -1379,7 +1379,29 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
13791379
}
13801380
}
13811381

1382-
fn check_foreign_fn(&mut self, def_id: LocalDefId, decl: &hir::FnDecl<'_>) {
1382+
/// Check if a function's argument types and result type are "ffi-safe".
1383+
///
1384+
/// For a external ABI function, argument types and the result type are walked to find fn-ptr
1385+
/// types that have external ABIs, as these still need checked.
1386+
fn check_fn(&mut self, def_id: LocalDefId, decl: &'tcx hir::FnDecl<'_>) {
1387+
let sig = self.cx.tcx.fn_sig(def_id).subst_identity();
1388+
let sig = self.cx.tcx.erase_late_bound_regions(sig);
1389+
1390+
for (input_ty, input_hir) in iter::zip(sig.inputs(), decl.inputs) {
1391+
for (fn_ptr_ty, span) in self.find_fn_ptr_ty_with_external_abi(input_hir, *input_ty) {
1392+
self.check_type_for_ffi_and_report_errors(span, fn_ptr_ty, false, false);
1393+
}
1394+
}
1395+
1396+
if let hir::FnRetTy::Return(ref ret_hir) = decl.output {
1397+
for (fn_ptr_ty, span) in self.find_fn_ptr_ty_with_external_abi(ret_hir, sig.output()) {
1398+
self.check_type_for_ffi_and_report_errors(span, fn_ptr_ty, false, true);
1399+
}
1400+
}
1401+
}
1402+
1403+
/// Check if a function's argument types and result type are "ffi-safe".
1404+
fn check_foreign_fn(&mut self, def_id: LocalDefId, decl: &'tcx hir::FnDecl<'_>) {
13831405
let sig = self.cx.tcx.fn_sig(def_id).subst_identity();
13841406
let sig = self.cx.tcx.erase_late_bound_regions(sig);
13851407

@@ -1388,8 +1410,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
13881410
}
13891411

13901412
if let hir::FnRetTy::Return(ref ret_hir) = decl.output {
1391-
let ret_ty = sig.output();
1392-
self.check_type_for_ffi_and_report_errors(ret_hir.span, ret_ty, false, true);
1413+
self.check_type_for_ffi_and_report_errors(ret_hir.span, sig.output(), false, true);
13931414
}
13941415
}
13951416

@@ -1404,28 +1425,131 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
14041425
SpecAbi::Rust | SpecAbi::RustCall | SpecAbi::RustIntrinsic | SpecAbi::PlatformIntrinsic
14051426
)
14061427
}
1428+
1429+
/// Find any fn-ptr types with external ABIs in `ty`.
1430+
///
1431+
/// For example, `Option<extern "C" fn()>` returns `extern "C" fn()`
1432+
fn find_fn_ptr_ty_with_external_abi(
1433+
&self,
1434+
hir_ty: &hir::Ty<'tcx>,
1435+
ty: Ty<'tcx>,
1436+
) -> Vec<(Ty<'tcx>, Span)> {
1437+
struct FnPtrFinder<'parent, 'a, 'tcx> {
1438+
visitor: &'parent ImproperCTypesVisitor<'a, 'tcx>,
1439+
spans: Vec<Span>,
1440+
tys: Vec<Ty<'tcx>>,
1441+
}
1442+
1443+
impl<'parent, 'a, 'tcx> hir::intravisit::Visitor<'_> for FnPtrFinder<'parent, 'a, 'tcx> {
1444+
fn visit_ty(&mut self, ty: &'_ hir::Ty<'_>) {
1445+
debug!(?ty);
1446+
if let hir::TyKind::BareFn(hir::BareFnTy { abi, .. }) = ty.kind
1447+
&& !self.visitor.is_internal_abi(*abi)
1448+
{
1449+
self.spans.push(ty.span);
1450+
}
1451+
1452+
hir::intravisit::walk_ty(self, ty)
1453+
}
1454+
}
1455+
1456+
impl<'vis, 'a, 'tcx> ty::visit::TypeVisitor<TyCtxt<'tcx>> for FnPtrFinder<'vis, 'a, 'tcx> {
1457+
type BreakTy = Ty<'tcx>;
1458+
1459+
fn visit_ty(&mut self, ty: Ty<'tcx>) -> ControlFlow<Self::BreakTy> {
1460+
if let ty::FnPtr(sig) = ty.kind() && !self.visitor.is_internal_abi(sig.abi()) {
1461+
self.tys.push(ty);
1462+
}
1463+
1464+
ty.super_visit_with(self)
1465+
}
1466+
}
1467+
1468+
let mut visitor = FnPtrFinder { visitor: &*self, spans: Vec::new(), tys: Vec::new() };
1469+
ty.visit_with(&mut visitor);
1470+
hir::intravisit::Visitor::visit_ty(&mut visitor, hir_ty);
1471+
1472+
iter::zip(visitor.tys.drain(..), visitor.spans.drain(..)).collect()
1473+
}
14071474
}
14081475

14091476
impl<'tcx> LateLintPass<'tcx> for ImproperCTypesDeclarations {
1410-
fn check_foreign_item(&mut self, cx: &LateContext<'_>, it: &hir::ForeignItem<'_>) {
1477+
fn check_foreign_item(&mut self, cx: &LateContext<'tcx>, it: &hir::ForeignItem<'tcx>) {
14111478
let mut vis = ImproperCTypesVisitor { cx, mode: CItemKind::Declaration };
14121479
let abi = cx.tcx.hir().get_foreign_abi(it.hir_id());
14131480

1414-
if !vis.is_internal_abi(abi) {
1415-
match it.kind {
1416-
hir::ForeignItemKind::Fn(ref decl, _, _) => {
1417-
vis.check_foreign_fn(it.owner_id.def_id, decl);
1418-
}
1419-
hir::ForeignItemKind::Static(ref ty, _) => {
1420-
vis.check_foreign_static(it.owner_id, ty.span);
1421-
}
1422-
hir::ForeignItemKind::Type => (),
1481+
match it.kind {
1482+
hir::ForeignItemKind::Fn(ref decl, _, _) if !vis.is_internal_abi(abi) => {
1483+
vis.check_foreign_fn(it.owner_id.def_id, decl);
1484+
}
1485+
hir::ForeignItemKind::Static(ref ty, _) if !vis.is_internal_abi(abi) => {
1486+
vis.check_foreign_static(it.owner_id, ty.span);
14231487
}
1488+
hir::ForeignItemKind::Fn(ref decl, _, _) => vis.check_fn(it.owner_id.def_id, decl),
1489+
hir::ForeignItemKind::Static(..) | hir::ForeignItemKind::Type => (),
1490+
}
1491+
}
1492+
}
1493+
1494+
impl ImproperCTypesDefinitions {
1495+
fn check_ty_maybe_containing_foreign_fnptr<'tcx>(
1496+
&mut self,
1497+
cx: &LateContext<'tcx>,
1498+
hir_ty: &'tcx hir::Ty<'_>,
1499+
ty: Ty<'tcx>,
1500+
) {
1501+
let mut vis = ImproperCTypesVisitor { cx, mode: CItemKind::Definition };
1502+
for (fn_ptr_ty, span) in vis.find_fn_ptr_ty_with_external_abi(hir_ty, ty) {
1503+
vis.check_type_for_ffi_and_report_errors(span, fn_ptr_ty, true, false);
14241504
}
14251505
}
14261506
}
14271507

1508+
/// `ImproperCTypesDefinitions` checks items outside of foreign items (e.g. stuff that isn't in
1509+
/// `extern "C" { }` blocks):
1510+
///
1511+
/// - `extern "<abi>" fn` definitions are checked in the same way as the
1512+
/// `ImproperCtypesDeclarations` visitor checks functions if `<abi>` is external (e.g. "C").
1513+
/// - All other items which contain types (e.g. other functions, struct definitions, etc) are
1514+
/// checked for extern fn-ptrs with external ABIs.
14281515
impl<'tcx> LateLintPass<'tcx> for ImproperCTypesDefinitions {
1516+
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'tcx>) {
1517+
match item.kind {
1518+
hir::ItemKind::Static(ty, ..)
1519+
| hir::ItemKind::Const(ty, ..)
1520+
| hir::ItemKind::TyAlias(ty, ..) => {
1521+
self.check_ty_maybe_containing_foreign_fnptr(
1522+
cx,
1523+
ty,
1524+
cx.tcx.type_of(item.owner_id).subst_identity(),
1525+
);
1526+
}
1527+
// See `check_fn`..
1528+
hir::ItemKind::Fn(..) => {}
1529+
// See `check_field_def`..
1530+
hir::ItemKind::Union(..) | hir::ItemKind::Struct(..) | hir::ItemKind::Enum(..) => {}
1531+
// Doesn't define something that can contain a external type to be checked.
1532+
hir::ItemKind::Impl(..)
1533+
| hir::ItemKind::TraitAlias(..)
1534+
| hir::ItemKind::Trait(..)
1535+
| hir::ItemKind::OpaqueTy(..)
1536+
| hir::ItemKind::GlobalAsm(..)
1537+
| hir::ItemKind::ForeignMod { .. }
1538+
| hir::ItemKind::Mod(..)
1539+
| hir::ItemKind::Macro(..)
1540+
| hir::ItemKind::Use(..)
1541+
| hir::ItemKind::ExternCrate(..) => {}
1542+
}
1543+
}
1544+
1545+
fn check_field_def(&mut self, cx: &LateContext<'tcx>, field: &'tcx hir::FieldDef<'tcx>) {
1546+
self.check_ty_maybe_containing_foreign_fnptr(
1547+
cx,
1548+
field.ty,
1549+
cx.tcx.type_of(field.def_id).subst_identity(),
1550+
);
1551+
}
1552+
14291553
fn check_fn(
14301554
&mut self,
14311555
cx: &LateContext<'tcx>,
@@ -1444,7 +1568,9 @@ impl<'tcx> LateLintPass<'tcx> for ImproperCTypesDefinitions {
14441568
};
14451569

14461570
let mut vis = ImproperCTypesVisitor { cx, mode: CItemKind::Definition };
1447-
if !vis.is_internal_abi(abi) {
1571+
if vis.is_internal_abi(abi) {
1572+
vis.check_fn(id, decl);
1573+
} else {
14481574
vis.check_foreign_fn(id, decl);
14491575
}
14501576
}

compiler/rustc_target/src/abi/call/x86_64.rs

+6-4
Original file line numberDiff line numberDiff line change
@@ -153,9 +153,9 @@ fn reg_component(cls: &[Option<Class>], i: &mut usize, size: Size) -> Option<Reg
153153
}
154154
}
155155

156-
fn cast_target(cls: &[Option<Class>], size: Size) -> CastTarget {
156+
fn cast_target(cls: &[Option<Class>], size: Size) -> Option<CastTarget> {
157157
let mut i = 0;
158-
let lo = reg_component(cls, &mut i, size).unwrap();
158+
let lo = reg_component(cls, &mut i, size)?;
159159
let offset = Size::from_bytes(8) * (i as u64);
160160
let mut target = CastTarget::from(lo);
161161
if size > offset {
@@ -164,7 +164,7 @@ fn cast_target(cls: &[Option<Class>], size: Size) -> CastTarget {
164164
}
165165
}
166166
assert_eq!(reg_component(cls, &mut i, Size::ZERO), None);
167-
target
167+
Some(target)
168168
}
169169

170170
const MAX_INT_REGS: usize = 6; // RDI, RSI, RDX, RCX, R8, R9
@@ -227,7 +227,9 @@ where
227227
// split into sized chunks passed individually
228228
if arg.layout.is_aggregate() {
229229
let size = arg.layout.size;
230-
arg.cast_to(cast_target(cls, size))
230+
if let Some(cast_target) = cast_target(cls, size) {
231+
arg.cast_to(cast_target);
232+
}
231233
} else {
232234
arg.extend_integer_width_to(32);
233235
}

tests/ui/abi/foreign/foreign-fn-with-byval.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
// run-pass
2-
#![allow(improper_ctypes)]
2+
#![allow(improper_ctypes, improper_ctypes_definitions)]
33

44
// ignore-wasm32-bare no libc to test ffi with
55

tests/ui/abi/issue-94223.rs

+8
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
// check-pass
2+
#![allow(improper_ctypes_definitions)]
3+
#![crate_type = "lib"]
4+
5+
// Check that computing the fn abi for `bad`, with a external ABI fn ptr that is not FFI-safe, does
6+
// not ICE.
7+
8+
pub fn bad(f: extern "C" fn([u8])) {}

tests/ui/hashmap/hashmap-memory.rs

+1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
// run-pass
22

3+
#![allow(improper_ctypes_definitions)]
34
#![allow(non_camel_case_types)]
45
#![allow(dead_code)]
56
#![allow(unused_mut)]

tests/ui/lint/lint-ctypes-94223.rs

+42
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
#![crate_type = "lib"]
2+
#![deny(improper_ctypes_definitions)]
3+
4+
pub fn bad(f: extern "C" fn([u8])) {}
5+
//~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe
6+
7+
pub fn bad_twice(f: Result<extern "C" fn([u8]), extern "C" fn([u8])>) {}
8+
//~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe
9+
//~^^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe
10+
11+
struct BadStruct(extern "C" fn([u8]));
12+
//~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe
13+
14+
enum BadEnum {
15+
A(extern "C" fn([u8])),
16+
//~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe
17+
}
18+
19+
enum BadUnion {
20+
A(extern "C" fn([u8])),
21+
//~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe
22+
}
23+
24+
type Foo = extern "C" fn([u8]);
25+
//~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe
26+
27+
pub struct FfiUnsafe;
28+
29+
#[allow(improper_ctypes_definitions)]
30+
extern "C" fn f(_: FfiUnsafe) {
31+
unimplemented!()
32+
}
33+
34+
pub static BAD: extern "C" fn(FfiUnsafe) = f;
35+
//~^ ERROR `extern` fn uses type `FfiUnsafe`, which is not FFI-safe
36+
37+
pub static BAD_TWICE: Result<extern "C" fn(FfiUnsafe), extern "C" fn(FfiUnsafe)> = Ok(f);
38+
//~^ ERROR `extern` fn uses type `FfiUnsafe`, which is not FFI-safe
39+
//~^^ ERROR `extern` fn uses type `FfiUnsafe`, which is not FFI-safe
40+
41+
pub const BAD_CONST: extern "C" fn(FfiUnsafe) = f;
42+
//~^ ERROR `extern` fn uses type `FfiUnsafe`, which is not FFI-safe

0 commit comments

Comments
 (0)