Auto merge of #65134 - davidtwco:issue-19834-improper-ctypes-in-exter…

…n-C-fn, r=rkruppe

improper_ctypes: `extern "C"` fns

cc #19834. Fixes #65867.

This pull request implements the change [described in this comment](#19834 (comment)).

cc @rkruppe @varkor @shepmaster

src/libpanic_abort/lib.rs

@@ -21,6 +21,7 @@
 // Rust's "try" function, but if we're aborting on panics we just call the
 // function as there's nothing else we need to do here.
 #[rustc_std_internal_symbol]
+#[allow(improper_ctypes)]
 pub unsafe extern fn __rust_maybe_catch_panic(f: fn(*mut u8),
                                               data: *mut u8,
                                               _data_ptr: *mut usize,

src/libpanic_unwind/lib.rs

@@ -69,6 +69,7 @@ mod dwarf;
 // hairy and tightly coupled, for more information see the compiler's
 // implementation of this.
 #[no_mangle]
+#[allow(improper_ctypes)]
 pub unsafe extern "C" fn __rust_maybe_catch_panic(f: fn(*mut u8),
                                                   data: *mut u8,
                                                   data_ptr: *mut usize,

src/librustc_codegen_llvm/back/write.rs

@@ -239,9 +239,7 @@ impl<'a> Drop for DiagnosticHandlers<'a> {
     }
 }
 
-unsafe extern "C" fn report_inline_asm(cgcx: &CodegenContext<LlvmCodegenBackend>,
-                                       msg: &str,
-                                       cookie: c_uint) {
+fn report_inline_asm(cgcx: &CodegenContext<LlvmCodegenBackend>, msg: &str, cookie: c_uint) {
     cgcx.diag_emitter.inline_asm_error(cookie as u32, msg.to_owned());
 }
 

src/librustc_codegen_llvm/llvm/mod.rs

@@ -87,6 +87,7 @@ pub struct RustString {
 }
 
 /// Appending to a Rust string -- used by RawRustStringOstream.
+#[allow(improper_ctypes)]
 #[no_mangle]
 pub unsafe extern "C" fn LLVMRustStringWriteImpl(sr: &RustString,
                                                  ptr: *const c_char,

src/librustc_lint/types.rs

@@ -4,7 +4,7 @@ use rustc::hir::{ExprKind, Node};
 use crate::hir::def_id::DefId;
 use rustc::hir::lowering::is_range_literal;
 use rustc::ty::subst::SubstsRef;
-use rustc::ty::{self, AdtKind, ParamEnv, Ty, TyCtxt};
+use rustc::ty::{self, AdtKind, ParamEnv, Ty, TyCtxt, TypeFoldable};
 use rustc::ty::layout::{self, IntegerExt, LayoutOf, VariantIdx, SizeSkeleton};
 use rustc::{lint, util};
 use rustc_index::vec::Idx;
@@ -835,16 +835,13 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
             ty::Array(ty, _) => self.check_type_for_ffi(cache, ty),
 
             ty::FnPtr(sig) => {
-                match sig.abi() {
-                    Abi::Rust | Abi::RustIntrinsic | Abi::PlatformIntrinsic | Abi::RustCall => {
-                        return FfiUnsafe {
-                            ty,
-                            reason: "this function pointer has Rust-specific calling convention",
-                            help: Some("consider using an `extern fn(...) -> ...` \
-                                        function pointer instead"),
-                        }
-                    }
-                    _ => {}
+                if self.is_internal_abi(sig.abi()) {
+                    return FfiUnsafe {
+                        ty,
+                        reason: "this function pointer has Rust-specific calling convention",
+                        help: Some("consider using an `extern fn(...) -> ...` \
+                                    function pointer instead"),
+                    };
                 }
 
                 let sig = cx.erase_late_bound_regions(&sig);
@@ -871,7 +868,10 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
 
             ty::Foreign(..) => FfiSafe,
 
-            ty::Param(..) |
+            // `extern "C" fn` functions can have type parameters, which may or may not be FFI-safe,
+            //  so they are currently ignored for the purposes of this lint, see #65134.
+            ty::Param(..) | ty::Projection(..) => FfiSafe,
+
             ty::Infer(..) |
             ty::Bound(..) |
             ty::Error |
@@ -880,7 +880,6 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
             ty::GeneratorWitness(..) |
             ty::Placeholder(..) |
             ty::UnnormalizedProjection(..) |
-            ty::Projection(..) |
             ty::Opaque(..) |
             ty::FnDef(..) => bug!("unexpected type in foreign function: {:?}", ty),
         }
@@ -892,11 +891,16 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         sp: Span,
         note: &str,
         help: Option<&str>,
+        is_foreign_item: bool,
     ) {
         let mut diag = self.cx.struct_span_lint(
             IMPROPER_CTYPES,
             sp,
-            &format!("`extern` block uses type `{}`, which is not FFI-safe", ty),
+            &format!(
+                "`extern` {} uses type `{}`, which is not FFI-safe",
+                if is_foreign_item { "block" } else { "fn" },
+                ty,
+            ),
         );
         diag.span_label(sp, "not FFI-safe");
         if let Some(help) = help {
@@ -911,9 +915,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         diag.emit();
     }
 
-    fn check_for_opaque_ty(&mut self, sp: Span, ty: Ty<'tcx>) -> bool {
-        use crate::rustc::ty::TypeFoldable;
-
+    fn check_for_opaque_ty(&mut self, sp: Span, ty: Ty<'tcx>, is_foreign_item: bool) -> bool {
         struct ProhibitOpaqueTypes<'tcx> {
             ty: Option<Ty<'tcx>>,
         };
@@ -937,70 +939,81 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                 sp,
                 "opaque types have no C equivalent",
                 None,
+                is_foreign_item,
             );
             true
         } else {
             false
         }
     }
 
-    fn check_type_for_ffi_and_report_errors(&mut self, sp: Span, ty: Ty<'tcx>) {
+    fn check_type_for_ffi_and_report_errors(
+        &mut self,
+        sp: Span,
+        ty: Ty<'tcx>,
+        is_foreign_item: bool,
+    ) {
         // We have to check for opaque types before `normalize_erasing_regions`,
         // which will replace opaque types with their underlying concrete type.
-        if self.check_for_opaque_ty(sp, ty) {
+        if self.check_for_opaque_ty(sp, ty, is_foreign_item) {
             // We've already emitted an error due to an opaque type.
             return;
         }
 
-        // it is only OK to use this function because extern fns cannot have
-        // any generic types right now:
-        let ty = self.cx.tcx.normalize_erasing_regions(ParamEnv::reveal_all(), ty);
-
+        let ty = self.cx.tcx.normalize_erasing_regions(self.cx.param_env, ty);
         match self.check_type_for_ffi(&mut FxHashSet::default(), ty) {
             FfiResult::FfiSafe => {}
             FfiResult::FfiPhantom(ty) => {
-                self.emit_ffi_unsafe_type_lint(ty, sp, "composed only of `PhantomData`", None);
+                self.emit_ffi_unsafe_type_lint(
+                    ty, sp, "composed only of `PhantomData`", None, is_foreign_item);
             }
             FfiResult::FfiUnsafe { ty, reason, help } => {
-                self.emit_ffi_unsafe_type_lint(ty, sp, reason, help);
+                self.emit_ffi_unsafe_type_lint(
+                    ty, sp, reason, help, is_foreign_item);
             }
         }
     }
 
-    fn check_foreign_fn(&mut self, id: hir::HirId, decl: &hir::FnDecl) {
+    fn check_foreign_fn(&mut self, id: hir::HirId, decl: &hir::FnDecl, is_foreign_item: bool) {
         let def_id = self.cx.tcx.hir().local_def_id(id);
         let sig = self.cx.tcx.fn_sig(def_id);
         let sig = self.cx.tcx.erase_late_bound_regions(&sig);
 
         for (input_ty, input_hir) in sig.inputs().iter().zip(&decl.inputs) {
-            self.check_type_for_ffi_and_report_errors(input_hir.span, input_ty);
+            self.check_type_for_ffi_and_report_errors(input_hir.span, input_ty, is_foreign_item);
         }
 
         if let hir::Return(ref ret_hir) = decl.output {
             let ret_ty = sig.output();
             if !ret_ty.is_unit() {
-                self.check_type_for_ffi_and_report_errors(ret_hir.span, ret_ty);
+                self.check_type_for_ffi_and_report_errors(ret_hir.span, ret_ty, is_foreign_item);
             }
         }
     }
 
     fn check_foreign_static(&mut self, id: hir::HirId, span: Span) {
         let def_id = self.cx.tcx.hir().local_def_id(id);
         let ty = self.cx.tcx.type_of(def_id);
-        self.check_type_for_ffi_and_report_errors(span, ty);
+        self.check_type_for_ffi_and_report_errors(span, ty, true);
+    }
+
+    fn is_internal_abi(&self, abi: Abi) -> bool {
+        if let Abi::Rust | Abi::RustCall | Abi::RustIntrinsic | Abi::PlatformIntrinsic = abi {
+            true
+        } else {
+            false
+        }
     }
 }
 
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ImproperCTypes {
     fn check_foreign_item(&mut self, cx: &LateContext<'_, '_>, it: &hir::ForeignItem) {
         let mut vis = ImproperCTypesVisitor { cx };
         let abi = cx.tcx.hir().get_foreign_abi(it.hir_id);
-        if let Abi::Rust | Abi::RustCall | Abi::RustIntrinsic | Abi::PlatformIntrinsic = abi {
-            // Don't worry about types in internal ABIs.
-        } else {
+        if !vis.is_internal_abi(abi) {
             match it.kind {
                 hir::ForeignItemKind::Fn(ref decl, _, _) => {
-                    vis.check_foreign_fn(it.hir_id, decl);
+                    vis.check_foreign_fn(it.hir_id, decl, true);
                 }
                 hir::ForeignItemKind::Static(ref ty, _) => {
                     vis.check_foreign_static(it.hir_id, ty.span);
@@ -1009,6 +1022,29 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ImproperCTypes {
             }
         }
     }
+
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'a, 'tcx>,
+        kind: hir::intravisit::FnKind<'tcx>,
+        decl: &'tcx hir::FnDecl,
+        _: &'tcx hir::Body,
+        _: Span,
+        hir_id: hir::HirId,
+    ) {
+        use hir::intravisit::FnKind;
+
+        let abi = match kind {
+            FnKind::ItemFn(_, _, header, ..) => (header.abi),
+            FnKind::Method(_, sig, ..) => (sig.header.abi),
+            _ => return,
+        };
+
+        let mut vis = ImproperCTypesVisitor { cx };
+        if !vis.is_internal_abi(abi) {
+            vis.check_foreign_fn(hir_id, decl, false);
+        }
+    }
 }
 
 declare_lint_pass!(VariantSizeDifferences => [VARIANT_SIZE_DIFFERENCES]);

src/test/ui/abi/abi-sysv64-register-usage.rs

@@ -33,6 +33,7 @@ pub extern "sysv64" fn all_the_registers(rdi: i64, rsi: i64, rdx: i64,
 
 // this struct contains 8 i64's, while only 6 can be passed in registers.
 #[cfg(target_arch = "x86_64")]
+#[repr(C)]
 #[derive(PartialEq, Eq, Debug)]
 pub struct LargeStruct(i64, i64, i64, i64, i64, i64, i64, i64);
 

src/test/ui/align-with-extern-c-fn.rs

@@ -7,7 +7,7 @@
 
 #![feature(repr_align)]
 
-#[repr(align(16))]
+#[repr(align(16), C)]
 pub struct A(i64);
 
 pub extern "C" fn foo(x: A) {}

src/test/ui/issues/issue-16441.rs

@@ -5,6 +5,7 @@
 struct Empty;
 
 // This used to cause an ICE
+#[allow(improper_ctypes)]
 extern "C" fn ice(_a: Empty) {}
 
 fn main() {

src/test/ui/issues/issue-26997.rs

@@ -6,6 +6,7 @@ pub struct Foo {
 }
 
 impl Foo {
+    #[allow(improper_ctypes)]
     pub extern fn foo_new() -> Foo {
         Foo { x: 21, y: 33 }
     }

src/test/ui/issues/issue-28600.rs

@@ -4,6 +4,7 @@
 struct Test;
 
 impl Test {
+    #[allow(improper_ctypes)]
     #[allow(dead_code)]
     #[allow(unused_variables)]
     pub extern fn test(val: &str) {

src/test/ui/issues/issue-38763.rs

@@ -4,6 +4,7 @@
 #[repr(C)]
 pub struct Foo(i128);
 
+#[allow(improper_ctypes)]
 #[no_mangle]
 pub extern "C" fn foo(x: Foo) -> Foo { x }
 

src/test/ui/issues/issue-51907.rs

@@ -6,7 +6,9 @@ trait Foo {
 
 struct Bar;
 impl Foo for Bar {
+    #[allow(improper_ctypes)]
     extern fn borrow(&self) {}
+    #[allow(improper_ctypes)]
     extern fn take(self: Box<Self>) {}
 }