Increase the precision of the exportable MIR check

Author: saethlin

compiler/rustc_metadata/src/rmeta/encoder.rs

@@ -1554,6 +1554,13 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
             self.tables.unused_generic_params.set(def_id.local_def_index, unused);
         }
 
+        // Encode promoted_mir for all functions that we inlined
+        let prev_len = tcx.inlined_internal_defs.lock().len();
+        for def_id in tcx.inlined_internal_defs.lock().clone() {
+            record!(self.tables.promoted_mir[def_id.to_def_id()] <- tcx.promoted_mir(def_id));
+        }
+        assert_eq!(tcx.inlined_internal_defs.lock().len(), prev_len);
+
         // Encode all the deduced parameter attributes for everything that has MIR, even for items
         // that can't be inlined. But don't if we aren't optimizing in non-incremental mode, to
         // save the query traffic.

compiler/rustc_middle/src/ty/context.rs

@@ -558,6 +558,8 @@ pub struct GlobalCtxt<'tcx> {
 
     /// Stores memory for globals (statics/consts).
     pub(crate) alloc_map: Lock<interpret::AllocMap<'tcx>>,
+
+    pub inlined_internal_defs: Lock<FxHashSet<LocalDefId>>,
 }
 
 impl<'tcx> GlobalCtxt<'tcx> {
@@ -706,6 +708,7 @@ impl<'tcx> TyCtxt<'tcx> {
             new_solver_evaluation_cache: Default::default(),
             data_layout,
             alloc_map: Lock::new(interpret::AllocMap::new()),
+            inlined_internal_defs: Default::default(),
         }
     }
 

compiler/rustc_mir_transform/src/inline.rs

@@ -146,6 +146,12 @@ impl<'tcx> Inliner<'tcx> {
                     debug!("inlined {}", callsite.callee);
                     self.changed = true;
 
+                    if !matches!(callsite.callee.def, InstanceDef::CloneShim(..)) {
+                        if let Some(def_id) = callsite.callee.def_id().as_local() {
+                            self.tcx.inlined_internal_defs.lock().insert(def_id);
+                        }
+                    }
+
                     self.history.push(callsite.callee.def_id());
                     self.process_blocks(caller_body, new_blocks);
                     self.history.pop();
@@ -184,6 +190,7 @@ impl<'tcx> Inliner<'tcx> {
 
         self.check_mir_is_available(caller_body, &callsite.callee)?;
         let callee_body = try_instance_mir(self.tcx, callsite.callee.def)?;
+        self.check_mir_is_exportable(&callsite.callee, callee_attrs, callee_body)?;
         self.check_mir_body(callsite, callee_body, callee_attrs)?;
 
         if !self.tcx.consider_optimizing(|| {
@@ -353,6 +360,44 @@ impl<'tcx> Inliner<'tcx> {
         None
     }
 
+    fn check_mir_is_exportable(
+        &self,
+        callee: &Instance<'tcx>,
+        callee_attrs: &CodegenFnAttrs,
+        callee_body: &Body<'tcx>,
+    ) -> Result<(), &'static str> {
+        // If the callee is not local, then it must be exportable because we passed the check for
+        // if MIR is available.
+        if !callee.def_id().is_local() {
+            return Ok(());
+        }
+        if self.tcx.is_constructor(callee.def_id()) {
+            return Ok(());
+        }
+        if callee_attrs.requests_inline() {
+            return Ok(());
+        }
+        let is_generic = callee.substs.non_erasable_generics().next().is_some();
+        if is_generic {
+            return Ok(());
+        }
+
+        // So now we are trying to inline a function from another crate which is not inline and is
+        // not a generic. This might work. But if this pulls in a symbol from the other crater, we
+        // will fail to link.
+        // So this is our heuritic for handling this:
+        // If this function has any calls in it, that might reference an internal symbol.
+        // If this function contains a pointer constant, that might be a reference to an internal
+        // static.
+        // So if either of those conditions are met, we cannot inline this.
+        if !contains_pointer_constant(callee_body) {
+            debug!("Has no pointer constants, must be exportable");
+            return Ok(());
+        }
+
+        Err("not exported")
+    }
+
     /// Returns an error if inlining is not possible based on codegen attributes alone. A success
     /// indicates that inlining decision should be based on other criteria.
     fn check_codegen_attributes(
@@ -364,16 +409,6 @@ impl<'tcx> Inliner<'tcx> {
             return Err("never inline hint");
         }
 
-        // Only inline local functions if they would be eligible for cross-crate
-        // inlining. This is to ensure that the final crate doesn't have MIR that
-        // reference unexported symbols
-        if callsite.callee.def_id().is_local() {
-            let is_generic = callsite.callee.substs.non_erasable_generics().next().is_some();
-            if !is_generic && !callee_attrs.requests_inline() {
-                return Err("not exported");
-            }
-        }
-
         if callsite.fn_sig.c_variadic() {
             return Err("C variadic");
         }
@@ -1148,3 +1183,41 @@ fn try_instance_mir<'tcx>(
         _ => Ok(tcx.instance_mir(instance)),
     }
 }
+
+fn contains_pointer_constant(body: &Body<'_>) -> bool {
+    let mut finder = ConstFinder { found: false };
+    finder.visit_body(body);
+    finder.found
+}
+
+struct ConstFinder {
+    found: bool,
+}
+
+impl Visitor<'_> for ConstFinder {
+    fn visit_constant(&mut self, constant: &Constant<'_>, location: Location) {
+        debug!("Visiting constant: {:?}", constant.literal);
+        if let ConstantKind::Unevaluated(..) = constant.literal {
+            self.found = true;
+            return;
+        }
+
+        if let ConstantKind::Val(val, ty) = constant.literal {
+            ty::tls::with(|tcx| {
+                let val = tcx.lift(val).unwrap();
+                if let ConstValue::Scalar(Scalar::Ptr(ptr, _size)) = val {
+                    if let Some(GlobalAlloc::Static(_)) =
+                        tcx.try_get_global_alloc(ptr.into_parts().0)
+                    {
+                        self.found = true;
+                    }
+                }
+            });
+
+            if ty.is_fn() {
+                self.found = true;
+            }
+        }
+        self.super_constant(constant, location);
+    }
+}

tests/codegen-units/item-collection/items-within-generic-items.rs

@@ -3,6 +3,7 @@
 #![deny(dead_code)]
 #![feature(start)]
 
+#[inline(never)]
 fn generic_fn<T>(a: T) -> (T, i32) {
     //~ MONO_ITEM fn generic_fn::nested_fn
     fn nested_fn(a: i32) -> i32 {

tests/codegen/call-metadata.rs

@@ -12,6 +12,7 @@ pub fn test() {
 }
 
 #[no_mangle]
+#[inline(never)]
 fn some_true() -> Option<bool> {
     Some(true)
 }

tests/codegen/cold-call-declare-and-call.rs

@@ -9,6 +9,7 @@
 // CHECK: call coldcc void @this_should_never_happen(i16
 
 #[no_mangle]
+#[inline(never)]
 pub extern "rust-cold" fn this_should_never_happen(x: u16) {}
 
 pub fn do_things(x: u16) {

tests/codegen/drop.rs

@@ -11,6 +11,7 @@ impl Drop for SomeUniqueName {
     }
 }
 
+#[inline(never)]
 pub fn possibly_unwinding() {
 }
 

tests/codegen/personality_lifetimes.rs

@@ -13,6 +13,7 @@ impl Drop for S {
     }
 }
 
+#[inline(never)]
 fn might_unwind() {
 }
 

tests/codegen/target-cpu-on-functions.rs

@@ -15,7 +15,8 @@ pub extern "C" fn exported() {
 
 // CHECK-LABEL: ; target_cpu_on_functions::not_exported
 // CHECK-NEXT: ; Function Attrs:
-// CHECK-NEXT: define {{.*}}() {{.*}} #0
+// CHECK-NEXT: define {{.*}}() {{.*}} #1
+#[inline(never)]
 fn not_exported() {}
 
 // CHECK: attributes #0 = {{.*}} "target-cpu"="{{.*}}"

tests/codegen/tune-cpu-on-functions.rs

@@ -15,7 +15,8 @@ pub extern fn exported() {
 
 // CHECK-LABEL: ; tune_cpu_on_functions::not_exported
 // CHECK-NEXT: ; Function Attrs:
-// CHECK-NEXT: define {{.*}}() {{.*}} #0
+// CHECK-NEXT: define {{.*}}() {{.*}} #1
+#[inline(never)]
 fn not_exported() {}
 
 // CHECK: attributes #0 = {{.*}} "tune-cpu"="{{.*}}"