Don't infer attributes of virtual calls based on the function body

Author: dianqk

compiler/rustc_middle/src/ty/instance.rs

@@ -111,8 +111,9 @@ pub enum InstanceKind<'tcx> {
 
     /// Dynamic dispatch to `<dyn Trait as Trait>::fn`.
     ///
-    /// This `InstanceKind` does not have callable MIR. Calls to `Virtual` instances must be
-    /// codegen'd as virtual calls through the vtable.
+    /// This `InstanceKind` may have a callable MIR as the default implementation.
+    /// Calls to `Virtual` instances must be codegen'd as virtual calls through the vtable.
+    /// *This means we might not know exactly what is being called.*
     ///
     /// If this is reified to a `fn` pointer, a `ReifyShim` is used (see `ReifyShim` above for more
     /// details on that).

compiler/rustc_ty_utils/src/abi.rs

@@ -309,15 +309,11 @@ fn fn_abi_of_fn_ptr<'tcx>(
     query: ty::PseudoCanonicalInput<'tcx, (ty::PolyFnSig<'tcx>, &'tcx ty::List<Ty<'tcx>>)>,
 ) -> Result<&'tcx FnAbi<'tcx, Ty<'tcx>>, &'tcx FnAbiError<'tcx>> {
     let ty::PseudoCanonicalInput { typing_env, value: (sig, extra_args) } = query;
-
-    let cx = LayoutCx::new(tcx, typing_env);
     fn_abi_new_uncached(
-        &cx,
+        &LayoutCx::new(tcx, typing_env),
         tcx.instantiate_bound_regions_with_erased(sig),
         extra_args,
         None,
-        None,
-        false,
     )
 }
 
@@ -326,19 +322,11 @@ fn fn_abi_of_instance<'tcx>(
     query: ty::PseudoCanonicalInput<'tcx, (ty::Instance<'tcx>, &'tcx ty::List<Ty<'tcx>>)>,
 ) -> Result<&'tcx FnAbi<'tcx, Ty<'tcx>>, &'tcx FnAbiError<'tcx>> {
     let ty::PseudoCanonicalInput { typing_env, value: (instance, extra_args) } = query;
-
-    let sig = fn_sig_for_fn_abi(tcx, instance, typing_env);
-
-    let caller_location =
-        instance.def.requires_caller_location(tcx).then(|| tcx.caller_location_ty());
-
     fn_abi_new_uncached(
         &LayoutCx::new(tcx, typing_env),
-        sig,
+        fn_sig_for_fn_abi(tcx, instance, typing_env),
         extra_args,
-        caller_location,
-        Some(instance.def_id()),
-        matches!(instance.def, ty::InstanceKind::Virtual(..)),
+        Some(instance),
     )
 }
 
@@ -547,19 +535,23 @@ fn fn_abi_sanity_check<'tcx>(
     fn_arg_sanity_check(cx, fn_abi, spec_abi, &fn_abi.ret);
 }
 
-// FIXME(eddyb) perhaps group the signature/type-containing (or all of them?)
-// arguments of this method, into a separate `struct`.
-#[tracing::instrument(level = "debug", skip(cx, caller_location, fn_def_id, force_thin_self_ptr))]
+#[tracing::instrument(level = "debug", skip(cx, instance))]
 fn fn_abi_new_uncached<'tcx>(
     cx: &LayoutCx<'tcx>,
     sig: ty::FnSig<'tcx>,
     extra_args: &[Ty<'tcx>],
-    caller_location: Option<Ty<'tcx>>,
-    fn_def_id: Option<DefId>,
-    // FIXME(eddyb) replace this with something typed, like an `enum`.
-    force_thin_self_ptr: bool,
+    instance: Option<ty::Instance<'tcx>>,
 ) -> Result<&'tcx FnAbi<'tcx, Ty<'tcx>>, &'tcx FnAbiError<'tcx>> {
     let tcx = cx.tcx();
+    let (caller_location, fn_def_id, is_virtual_call) = if let Some(instance) = instance {
+        (
+            instance.def.requires_caller_location(tcx).then(|| tcx.caller_location_ty()),
+            Some(instance.def_id()),
+            matches!(instance.def, ty::InstanceKind::Virtual(..)),
+        )
+    } else {
+        (None, None, false)
+    };
     let sig = tcx.normalize_erasing_regions(cx.typing_env, sig);
 
     let conv = conv_from_spec_abi(cx.tcx(), sig.abi, sig.c_variadic);
@@ -603,7 +595,7 @@ fn fn_abi_new_uncached<'tcx>(
         });
 
         let layout = cx.layout_of(ty).map_err(|err| &*tcx.arena.alloc(FnAbiError::Layout(*err)))?;
-        let layout = if force_thin_self_ptr && arg_idx == Some(0) {
+        let layout = if is_virtual_call && arg_idx == Some(0) {
             // Don't pass the vtable, it's not an argument of the virtual fn.
             // Instead, pass just the data pointer, but give it the type `*const/mut dyn Trait`
             // or `&/&mut dyn Trait` because this is special-cased elsewhere in codegen
@@ -648,7 +640,15 @@ fn fn_abi_new_uncached<'tcx>(
         conv,
         can_unwind: fn_can_unwind(cx.tcx(), fn_def_id, sig.abi),
     };
-    fn_abi_adjust_for_abi(cx, &mut fn_abi, sig.abi, fn_def_id);
+    fn_abi_adjust_for_abi(
+        cx,
+        &mut fn_abi,
+        sig.abi,
+        // If this is a virtual call, we cannot pass the `fn_def_id`, as it might call other
+        // functions from vtable. Internally, `deduced_param_attrs` attempts to infer attributes by
+        // visit the function body.
+        fn_def_id.filter(|_| !is_virtual_call),
+    );
     debug!("fn_abi_new_uncached = {:?}", fn_abi);
     fn_abi_sanity_check(cx, &fn_abi, sig.abi);
     Ok(tcx.arena.alloc(fn_abi))

tests/codegen/virtual-call-attrs-issue-137646.rs

@@ -0,0 +1,18 @@
+//! Regression test for https://github.com/rust-lang/rust/issues/137646.
+//! Since we don't know the exact implementation of the virtual call,
+//! it might write to parameters, we can't infer the readonly attribute.
+//@ compile-flags: -C opt-level=3 -C no-prepopulate-passes
+
+#![crate_type = "lib"]
+
+pub trait Trait {
+    fn m(&self, _: (i32, i32, i32)) {}
+}
+
+#[no_mangle]
+pub fn foo(trait_: &dyn Trait) {
+    // CHECK-LABEL: @foo(
+    // CHECK: call void
+    // CHECK-NOT: readonly
+    trait_.m((1, 1, 1));
+}

tests/ui/virtual-call-attrs-issue-137646.rs

@@ -1,8 +1,7 @@
 //! Regression test for https://github.com/rust-lang/rust/issues/137646.
-//! FIXME: The parameter value at all calls to `check` should be `(1, 1, 1)`.
+//! The parameter value at all calls to `check` should be `(1, 1, 1)`.
 
 //@ run-pass
-//@ check-run-results
 
 use std::hint::black_box;
 
@@ -35,8 +34,7 @@ pub fn run_2(trait_: &dyn Trait) {
 
 #[inline(never)]
 fn check(v: T) {
-    dbg!(v);
-    // assert_eq!(v, (1, 1, 1));
+    assert_eq!(v, (1, 1, 1));
 }
 
 fn main() {