From fb55415161194a4740574dedc2f8663b4db2a5b3 Mon Sep 17 00:00:00 2001 From: Alexa VanHattum Date: Wed, 21 Jul 2021 16:23:17 +0000 Subject: [PATCH 1/2] Use passed vtable index in resolving functions --- .../rustc_codegen_llvm/src/gotoc/metadata.rs | 19 +++++---- .../rustc_codegen_llvm/src/gotoc/rvalue.rs | 8 ++-- .../rustc_codegen_llvm/src/gotoc/statement.rs | 6 ++- compiler/rustc_codegen_llvm/src/gotoc/typ.rs | 10 +++-- .../cbmc/DynTrait/generic_duplicate_names.rs | 26 +++++++++++++ .../DynTrait/generic_duplicate_names_fail.rs | 29 ++++++++++++++ .../DynTrait/generic_duplicate_names_impl.rs | 34 ++++++++++++++++ .../generic_duplicate_names_impl_fail.rs | 37 ++++++++++++++++++ .../cbmc/DynTrait/object_safe_generics.rs | 39 +++++++++++++++++++ 9 files changed, 189 insertions(+), 19 deletions(-) create mode 100644 src/test/cbmc/DynTrait/generic_duplicate_names.rs create mode 100644 src/test/cbmc/DynTrait/generic_duplicate_names_fail.rs create mode 100644 src/test/cbmc/DynTrait/generic_duplicate_names_impl.rs create mode 100644 src/test/cbmc/DynTrait/generic_duplicate_names_impl_fail.rs create mode 100644 src/test/cbmc/DynTrait/object_safe_generics.rs diff --git a/compiler/rustc_codegen_llvm/src/gotoc/metadata.rs b/compiler/rustc_codegen_llvm/src/gotoc/metadata.rs index b5f59c7f71a6..d48fa53e79ce 100644 --- a/compiler/rustc_codegen_llvm/src/gotoc/metadata.rs +++ b/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. diff --git a/compiler/rustc_codegen_llvm/src/gotoc/rvalue.rs b/compiler/rustc_codegen_llvm/src/gotoc/rvalue.rs index fcd89e739dea..91793c127e99 100644 --- a/compiler/rustc_codegen_llvm/src/gotoc/rvalue.rs +++ b/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,7 +850,8 @@ impl<'tcx> GotocCtx<'tcx> { let vtable_fields: Vec = 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)) } @@ -857,7 +859,7 @@ impl<'tcx> GotocCtx<'tcx> { 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(); diff --git a/compiler/rustc_codegen_llvm/src/gotoc/statement.rs b/compiler/rustc_codegen_llvm/src/gotoc/statement.rs index 82347dbaa34f..b3d6e661504a 100644 --- a/compiler/rustc_codegen_llvm/src/gotoc/statement.rs +++ b/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, loc: Location, ) -> Vec { - 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 diff --git a/compiler/rustc_codegen_llvm/src/gotoc/typ.rs b/compiler/rustc_codegen_llvm/src/gotoc/typ.rs index 1328ae54a5fd..203278eaef37 100644 --- a/compiler/rustc_codegen_llvm/src/gotoc/typ.rs +++ b/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 diff --git a/src/test/cbmc/DynTrait/generic_duplicate_names.rs b/src/test/cbmc/DynTrait/generic_duplicate_names.rs new file mode 100644 index 000000000000..114925c97713 --- /dev/null +++ b/src/test/cbmc/DynTrait/generic_duplicate_names.rs @@ -0,0 +1,26 @@ +// 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 { + fn method(&self, t: T) -> T; +} + +trait Bar: Foo + Foo { } + +impl Foo 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 and one for Foo. + let result = >::method(b, 22_u32); + assert!(result == 22_u32); +} diff --git a/src/test/cbmc/DynTrait/generic_duplicate_names_fail.rs b/src/test/cbmc/DynTrait/generic_duplicate_names_fail.rs new file mode 100644 index 000000000000..bb4376213edd --- /dev/null +++ b/src/test/cbmc/DynTrait/generic_duplicate_names_fail.rs @@ -0,0 +1,29 @@ +// 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 { + fn method(&self, t: T) -> T; +} + +trait Bar: Foo + Foo { } + +impl Foo 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 and one for Foo. + let result = >::method(b, 22_u32); + __VERIFIER_expect_fail(result == 0, "Wrong result"); +} diff --git a/src/test/cbmc/DynTrait/generic_duplicate_names_impl.rs b/src/test/cbmc/DynTrait/generic_duplicate_names_impl.rs new file mode 100644 index 000000000000..ab3d626446e8 --- /dev/null +++ b/src/test/cbmc/DynTrait/generic_duplicate_names_impl.rs @@ -0,0 +1,34 @@ +// 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 { + fn method(&self, t: T) -> T; +} + +trait Bar: Foo + Foo { } + +impl Foo for () { + fn method(&self, t: i32) -> i32 { 0 } +} + +impl Foo 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 and one for Foo. + let result = >::method(b, 22_u32); + assert!(result == 22_u32); + + let result = >::method(b, 22_i32); + assert!(result == 0); +} diff --git a/src/test/cbmc/DynTrait/generic_duplicate_names_impl_fail.rs b/src/test/cbmc/DynTrait/generic_duplicate_names_impl_fail.rs new file mode 100644 index 000000000000..b1810063a169 --- /dev/null +++ b/src/test/cbmc/DynTrait/generic_duplicate_names_impl_fail.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. This _fail test +// expects the wrong result. + +include!("../../rmc-prelude.rs"); + +trait Foo { + fn method(&self, t: T) -> T; +} + +trait Bar: Foo + Foo { } + +impl Foo for () { + fn method(&self, t: i32) -> i32 { 0 } +} + +impl Foo 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 and one for Foo. + let result = >::method(b, 22_u32); + __VERIFIER_expect_fail(result == 0, "Wrong function"); + + let result = >::method(b, 22_i32); + __VERIFIER_expect_fail(result == 22_i32, "Wrong function"); +} diff --git a/src/test/cbmc/DynTrait/object_safe_generics.rs b/src/test/cbmc/DynTrait/object_safe_generics.rs new file mode 100644 index 000000000000..e6e8efdb52d1 --- /dev/null +++ b/src/test/cbmc/DynTrait/object_safe_generics.rs @@ -0,0 +1,39 @@ +// 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 { + // 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 + Foo { } + +impl Foo 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 and one for Foo. Both follow the + // vacant vtable entries for Foo::new and Foo::new + // which are not object safe. + let result = >::method(b, 22_u32); + assert!(result == 22_u32); +} From 099bc5c9645391b8a3895eb88e5bf9749e2d3191 Mon Sep 17 00:00:00 2001 From: Alexa VanHattum Date: Wed, 21 Jul 2021 19:06:34 +0000 Subject: [PATCH 2/2] fmt --- .../cbmc/DynTrait/generic_duplicate_names.rs | 9 +++++---- .../DynTrait/generic_duplicate_names_fail.rs | 11 ++++++----- .../DynTrait/generic_duplicate_names_impl.rs | 13 ++++++++----- .../generic_duplicate_names_impl_fail.rs | 13 ++++++++----- src/test/cbmc/DynTrait/object_safe_generics.rs | 16 +++++++++------- 5 files changed, 36 insertions(+), 26 deletions(-) diff --git a/src/test/cbmc/DynTrait/generic_duplicate_names.rs b/src/test/cbmc/DynTrait/generic_duplicate_names.rs index 114925c97713..7826f6ab16b0 100644 --- a/src/test/cbmc/DynTrait/generic_duplicate_names.rs +++ b/src/test/cbmc/DynTrait/generic_duplicate_names.rs @@ -8,14 +8,15 @@ trait Foo { fn method(&self, t: T) -> T; } -trait Bar: Foo + Foo { } +trait Bar: Foo + Foo {} impl Foo for () { - fn method(&self, t: T) -> T { t } + fn method(&self, t: T) -> T { + t + } } -impl Bar for () { -} +impl Bar for () {} fn main() { let b: &dyn Bar = &(); diff --git a/src/test/cbmc/DynTrait/generic_duplicate_names_fail.rs b/src/test/cbmc/DynTrait/generic_duplicate_names_fail.rs index bb4376213edd..521f1cf16c58 100644 --- a/src/test/cbmc/DynTrait/generic_duplicate_names_fail.rs +++ b/src/test/cbmc/DynTrait/generic_duplicate_names_fail.rs @@ -2,7 +2,7 @@ // 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 +// generic types give the same Trait::function name pairs. Test the // wrong result for this _fail test. include!("../../rmc-prelude.rs"); @@ -11,14 +11,15 @@ trait Foo { fn method(&self, t: T) -> T; } -trait Bar: Foo + Foo { } +trait Bar: Foo + Foo {} impl Foo for () { - fn method(&self, t: T) -> T { t } + fn method(&self, t: T) -> T { + t + } } -impl Bar for () { -} +impl Bar for () {} fn main() { let b: &dyn Bar = &(); diff --git a/src/test/cbmc/DynTrait/generic_duplicate_names_impl.rs b/src/test/cbmc/DynTrait/generic_duplicate_names_impl.rs index ab3d626446e8..5d542842c0f9 100644 --- a/src/test/cbmc/DynTrait/generic_duplicate_names_impl.rs +++ b/src/test/cbmc/DynTrait/generic_duplicate_names_impl.rs @@ -9,18 +9,21 @@ trait Foo { fn method(&self, t: T) -> T; } -trait Bar: Foo + Foo { } +trait Bar: Foo + Foo {} impl Foo for () { - fn method(&self, t: i32) -> i32 { 0 } + fn method(&self, t: i32) -> i32 { + 0 + } } impl Foo for () { - fn method(&self, t: u32) -> u32 { t } + fn method(&self, t: u32) -> u32 { + t + } } -impl Bar for () { -} +impl Bar for () {} fn main() { let b: &dyn Bar = &(); diff --git a/src/test/cbmc/DynTrait/generic_duplicate_names_impl_fail.rs b/src/test/cbmc/DynTrait/generic_duplicate_names_impl_fail.rs index b1810063a169..9a35a3f50191 100644 --- a/src/test/cbmc/DynTrait/generic_duplicate_names_impl_fail.rs +++ b/src/test/cbmc/DynTrait/generic_duplicate_names_impl_fail.rs @@ -12,18 +12,21 @@ trait Foo { fn method(&self, t: T) -> T; } -trait Bar: Foo + Foo { } +trait Bar: Foo + Foo {} impl Foo for () { - fn method(&self, t: i32) -> i32 { 0 } + fn method(&self, t: i32) -> i32 { + 0 + } } impl Foo for () { - fn method(&self, t: u32) -> u32 { t } + fn method(&self, t: u32) -> u32 { + t + } } -impl Bar for () { -} +impl Bar for () {} fn main() { let b: &dyn Bar = &(); diff --git a/src/test/cbmc/DynTrait/object_safe_generics.rs b/src/test/cbmc/DynTrait/object_safe_generics.rs index e6e8efdb52d1..1427151344ad 100644 --- a/src/test/cbmc/DynTrait/object_safe_generics.rs +++ b/src/test/cbmc/DynTrait/object_safe_generics.rs @@ -2,7 +2,7 @@ // 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 +// generic types give the same Trait::function name pairs, even when // non-object-safe methods force the vtable to have VtblEntry::Vacant // positions @@ -10,28 +10,30 @@ trait Foo { // Non-object-safe method first, so the vtable has // a vacant spot before the important method fn new() -> Self - where Self : Sized; + where + Self: Sized; fn method(&self, t: T) -> T; } -trait Bar: Foo + Foo { } +trait Bar: Foo + Foo {} impl Foo for () { fn new() -> Self { unimplemented!() } - fn method(&self, t: T) -> T { t } + fn method(&self, t: T) -> T { + t + } } -impl Bar for () { -} +impl Bar for () {} fn main() { let b: &dyn Bar = &(); // The vtable for b will now have two Foo::method entries, - // one for Foo and one for Foo. Both follow the + // one for Foo and one for Foo. Both follow the // vacant vtable entries for Foo::new and Foo::new // which are not object safe. let result = >::method(b, 22_u32);