Generics in dyn trait fix: use vtable index to resolve functions (rust-lang#352)

Use vtable index passed at call site to resolve functions

Co-authored-by: Daniel Schwartz-Narbonne <[email protected]>

Author: 2 people

compiler/rustc_codegen_llvm/src/gotoc/metadata.rs

@@ -92,16 +92,15 @@ impl<'tcx> GotocCtx<'tcx> {
         format!("{}::global::{}::", self.crate_name(), c)
     }
 
-    /// For the vtable field name, we need exactly the dyn trait name and the function
-    /// name. The table itself already is scoped by the object type.
-    ///     Example: ::Shape::vol
-    /// Note: this is _not_ the same name for top-level entry into the symbol table,
-    /// which does need more crate/type information and uses the full symbol_name(...)
-    pub fn vtable_field_name(&self, def_id: DefId) -> String {
-        // `to_string_no_crate_verbose` is from Rust proper, we use it here because it
-        // always includes the dyn trait name and function name.
-        // Tracking a less brittle solution here: https://github.com/model-checking/rmc/issues/187
-        self.tcx.def_path(def_id).to_string_no_crate_verbose()
+    /// The name for the struct field on a vtable for a given function. Because generic
+    /// functions can share the same name, we need to use the index of the entry in the
+    /// vtable. This is the same index that will be passed in virtual function calls as
+    /// InstanceDef::Virtual(def_id, idx). We could use solely the index as a key into
+    /// the vtable struct, but we add the trait and function names for debugging
+    /// readability.
+    ///     Example: 3_Shape::vol
+    pub fn vtable_field_name(&self, def_id: DefId, idx: usize) -> String {
+        format!("{}_{}", idx, with_no_trimmed_paths(|| self.tcx.def_path_str(def_id)))
     }
 
     /// A human readable name in Rust for reference, should not be used as a key.

compiler/rustc_codegen_llvm/src/gotoc/rvalue.rs

@@ -705,8 +705,9 @@ impl<'tcx> GotocCtx<'tcx> {
         def_id: DefId,
         substs: ty::subst::SubstsRef<'tcx>,
         t: Ty<'tcx>,
+        idx: usize,
     ) -> Expr {
-        let vtable_field_name = self.vtable_field_name(def_id);
+        let vtable_field_name = self.vtable_field_name(def_id, idx);
         let vtable_type_name = aggr_name(&self.vtable_name(t));
         let field_type = self
             .symbol_table
@@ -849,15 +850,16 @@ impl<'tcx> GotocCtx<'tcx> {
 
                 let vtable_fields: Vec<Expr> = vtable_entries
                     .iter()
-                    .filter_map(|entry| match entry {
+                    .enumerate()
+                    .filter_map(|(idx, entry)| match entry {
                         VtblEntry::MetadataDropInPlace => {
                             Some(ctx.codegen_vtable_drop_in_place(&src_mir_type, trait_type))
                         }
                         VtblEntry::MetadataSize => Some(vt_size.clone()),
                         VtblEntry::MetadataAlign => Some(vt_align.clone()),
                         VtblEntry::Vacant => None,
                         VtblEntry::Method(def_id, substs) => {
-                            Some(ctx.codegen_vtable_method_field(*def_id, substs, trait_type))
+                            Some(ctx.codegen_vtable_method_field(*def_id, substs, trait_type, idx))
                         }
                     })
                     .collect();

compiler/rustc_codegen_llvm/src/gotoc/statement.rs

@@ -283,7 +283,7 @@ impl<'tcx> GotocCtx<'tcx> {
                         return Stmt::goto(self.current_fn().find_label(&target), Location::none());
                     }
                     // Handle a virtual function call via a vtable lookup
-                    InstanceDef::Virtual(def_id, _) => {
+                    InstanceDef::Virtual(def_id, idx) => {
                         // We must have at least one argument, and the first one
                         // should be a fat pointer for the trait
                         let trait_fat_ptr = fargs[0].to_owned();
@@ -294,6 +294,7 @@ impl<'tcx> GotocCtx<'tcx> {
                         self.codegen_virtual_funcall(
                             trait_fat_ptr,
                             def_id,
+                            idx,
                             &p,
                             &mut fargs,
                             loc.clone(),
@@ -340,11 +341,12 @@ impl<'tcx> GotocCtx<'tcx> {
         &mut self,
         trait_fat_ptr: Expr,
         def_id: DefId,
+        idx: usize,
         place: &Place<'tcx>,
         fargs: &mut Vec<Expr>,
         loc: Location,
     ) -> Vec<Stmt> {
-        let vtable_field_name = self.vtable_field_name(def_id);
+        let vtable_field_name = self.vtable_field_name(def_id, idx);
 
         // Now that we have all the stuff we need, we can actually build the dynamic call
         // If the original call was of the form

compiler/rustc_codegen_llvm/src/gotoc/typ.rs

@@ -119,6 +119,7 @@ impl<'tcx> GotocCtx<'tcx> {
         &mut self,
         def_id: DefId,
         substs: SubstsRef<'tcx>,
+        idx: usize,
     ) -> DatatypeComponent {
         let instance = Instance::resolve(self.tcx, ty::ParamEnv::reveal_all(), def_id, substs)
             .unwrap()
@@ -130,8 +131,8 @@ impl<'tcx> GotocCtx<'tcx> {
         // gives an Irep Pointer object for the signature
         let fnptr = self.codegen_dynamic_function_sig(sig).to_pointer();
 
-        // vtable field name, i.e., ::Shape::vol
-        let vtable_field_name = self.vtable_field_name(def_id);
+        // vtable field name, i.e., 3_Shape::vol (idx_Trait::method)
+        let vtable_field_name = self.vtable_field_name(def_id, idx);
 
         let ins_ty = instance.ty(self.tcx, ty::ParamEnv::reveal_all());
         let _layout = self.layout_of(ins_ty);
@@ -195,9 +196,10 @@ impl<'tcx> GotocCtx<'tcx> {
                     .vtable_entries(poly)
                     .iter()
                     .cloned()
-                    .filter_map(|entry| match entry {
+                    .enumerate()
+                    .filter_map(|(idx, entry)| match entry {
                         VtblEntry::Method(def_id, substs) => {
-                            Some(self.trait_method_vtable_field_type(def_id, substs))
+                            Some(self.trait_method_vtable_field_type(def_id, substs, idx))
                         }
                         VtblEntry::MetadataDropInPlace
                         | VtblEntry::MetadataSize

src/test/cbmc/DynTrait/generic_duplicate_names.rs

@@ -0,0 +1,27 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0 OR MIT
+
+// This test checks that we can handle potential naming conflicts when
+// generic types give the same Trait::function name pairs.
+
+trait Foo<T> {
+    fn method(&self, t: T) -> T;
+}
+
+trait Bar: Foo<u32> + Foo<i32> {}
+
+impl<T> Foo<T> for () {
+    fn method(&self, t: T) -> T {
+        t
+    }
+}
+
+impl Bar for () {}
+
+fn main() {
+    let b: &dyn Bar = &();
+    // The vtable for b will now have two Foo::method entries,
+    // one for Foo<u32> and one for Foo<i32>.
+    let result = <dyn Bar as Foo<u32>>::method(b, 22_u32);
+    assert!(result == 22_u32);
+}

src/test/cbmc/DynTrait/generic_duplicate_names_fail.rs

@@ -0,0 +1,30 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0 OR MIT
+
+// This test checks that we can handle potential naming conflicts when
+// generic types give the same Trait::function name pairs. Test the
+// wrong result for this _fail test.
+
+include!("../../rmc-prelude.rs");
+
+trait Foo<T> {
+    fn method(&self, t: T) -> T;
+}
+
+trait Bar: Foo<u32> + Foo<i32> {}
+
+impl<T> Foo<T> for () {
+    fn method(&self, t: T) -> T {
+        t
+    }
+}
+
+impl Bar for () {}
+
+fn main() {
+    let b: &dyn Bar = &();
+    // The vtable for b will now have two Foo::method entries,
+    // one for Foo<u32> and one for Foo<i32>.
+    let result = <dyn Bar as Foo<u32>>::method(b, 22_u32);
+    __VERIFIER_expect_fail(result == 0, "Wrong result");
+}

src/test/cbmc/DynTrait/generic_duplicate_names_impl.rs

@@ -0,0 +1,37 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0 OR MIT
+
+// This test checks that we can handle potential naming conflicts when
+// generic types give the same Trait::function name pairs. This test
+// gives different concrete impls for i32 and u32.
+
+trait Foo<T> {
+    fn method(&self, t: T) -> T;
+}
+
+trait Bar: Foo<u32> + Foo<i32> {}
+
+impl Foo<i32> for () {
+    fn method(&self, t: i32) -> i32 {
+        0
+    }
+}
+
+impl Foo<u32> for () {
+    fn method(&self, t: u32) -> u32 {
+        t
+    }
+}
+
+impl Bar for () {}
+
+fn main() {
+    let b: &dyn Bar = &();
+    // The vtable for b will now have two Foo::method entries,
+    // one for Foo<u32> and one for Foo<i32>.
+    let result = <dyn Bar as Foo<u32>>::method(b, 22_u32);
+    assert!(result == 22_u32);
+
+    let result = <dyn Bar as Foo<i32>>::method(b, 22_i32);
+    assert!(result == 0);
+}

src/test/cbmc/DynTrait/generic_duplicate_names_impl_fail.rs

@@ -0,0 +1,40 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0 OR MIT
+
+// This test checks that we can handle potential naming conflicts when
+// generic types give the same Trait::function name pairs. This test
+// gives different concrete impls for i32 and u32. This _fail test
+// expects the wrong result.
+
+include!("../../rmc-prelude.rs");
+
+trait Foo<T> {
+    fn method(&self, t: T) -> T;
+}
+
+trait Bar: Foo<u32> + Foo<i32> {}
+
+impl Foo<i32> for () {
+    fn method(&self, t: i32) -> i32 {
+        0
+    }
+}
+
+impl Foo<u32> for () {
+    fn method(&self, t: u32) -> u32 {
+        t
+    }
+}
+
+impl Bar for () {}
+
+fn main() {
+    let b: &dyn Bar = &();
+    // The vtable for b will now have two Foo::method entries,
+    // one for Foo<u32> and one for Foo<i32>.
+    let result = <dyn Bar as Foo<u32>>::method(b, 22_u32);
+    __VERIFIER_expect_fail(result == 0, "Wrong function");
+
+    let result = <dyn Bar as Foo<i32>>::method(b, 22_i32);
+    __VERIFIER_expect_fail(result == 22_i32, "Wrong function");
+}

src/test/cbmc/DynTrait/object_safe_generics.rs

@@ -0,0 +1,41 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0 OR MIT
+
+// This test checks that we can handle potential naming conflicts when
+// generic types give the same Trait::function name pairs, even when
+// non-object-safe methods force the vtable to have VtblEntry::Vacant
+// positions
+
+trait Foo<T> {
+    // Non-object-safe method first, so the vtable has
+    // a vacant spot before the important method
+    fn new() -> Self
+    where
+        Self: Sized;
+
+    fn method(&self, t: T) -> T;
+}
+
+trait Bar: Foo<u32> + Foo<i32> {}
+
+impl<T> Foo<T> for () {
+    fn new() -> Self {
+        unimplemented!()
+    }
+
+    fn method(&self, t: T) -> T {
+        t
+    }
+}
+
+impl Bar for () {}
+
+fn main() {
+    let b: &dyn Bar = &();
+    // The vtable for b will now have two Foo::method entries,
+    // one for Foo<u32> and one for Foo<i32>. Both follow the
+    // vacant vtable entries for Foo<u32>::new and Foo<i32>::new
+    // which are not object safe.
+    let result = <dyn Bar as Foo<u32>>::method(b, 22_u32);
+    assert!(result == 22_u32);
+}