Respond to code review of unsized pointer cast implementation

compiler/rustc_codegen_llvm/src/gotoc/cbmc/goto_program/expr.rs

@@ -1206,25 +1206,17 @@ impl Expr {
     }
 }
 
-/// Use maps instead of lists to manage struct components.
 impl Expr {
-    /// A mapping from field names to field types (ignoring padding fields)
-    pub fn struct_field_types(&self, symbol_table: &SymbolTable) -> BTreeMap<String, Type> {
-        let struct_type = self.typ();
-        assert!(struct_type.is_struct_tag());
-
-        let mut types: BTreeMap<String, Type> = BTreeMap::new();
-        let fields = symbol_table.lookup_fields_in_type(struct_type).unwrap();
-        for field in fields {
-            if field.is_padding() {
-                continue;
-            }
-            types.insert(field.name().to_string(), field.typ());
-        }
-        types
-    }
-
-    /// A mapping from field names to field exprs (ignoring padding fields)
+    /// Given a struct value (Expr), construct a mapping from struct field names
+    /// (Strings) to struct field values (Exprs).
+    ///
+    /// The Struct variant of the Expr enum models the fields of a struct as a
+    /// list of pairs (data type components) consisting of a field name and a
+    /// field value.  A pair may represent an actual field in the struct or just
+    /// padding in the layout of the struct.  This function returns a mapping of
+    /// field names (ignoring the padding fields) to field values.  The result
+    /// is suitable for use in the struct_expr constructor.  This makes it
+    /// easier to look up or modify field values of a struct.
     pub fn struct_field_exprs(&self, symbol_table: &SymbolTable) -> BTreeMap<String, Expr> {
         let struct_type = self.typ();
         assert!(struct_type.is_struct_tag());

compiler/rustc_codegen_llvm/src/gotoc/cbmc/goto_program/typ.rs

@@ -5,6 +5,7 @@ use self::Type::*;
 use super::super::utils::aggr_name;
 use super::super::MachineModel;
 use super::{Expr, SymbolTable};
+use std::collections::BTreeMap;
 use std::convert::TryInto;
 use std::fmt::Debug;
 
@@ -782,3 +783,30 @@ impl Type {
         }
     }
 }
+
+impl Type {
+    /// Given a struct type, construct a mapping from struct field names
+    /// (Strings) to struct field types (Types).
+    ///
+    /// The Struct variant of the Type enum models the fields of a struct as a
+    /// list of pairs (data type components) consisting of a field name and a
+    /// field type.  A pair may represent an actual field in the struct or just
+    /// padding in the layout of the struct.  This function returns a mapping of
+    /// field names (ignoring the padding fields) to field types.  This makes it
+    /// easier to look up field types (and modestly easier to interate over
+    /// field types).
+    pub fn struct_field_types(&self, symbol_table: &SymbolTable) -> BTreeMap<String, Type> {
+        // TODO: Accept a Struct type, too, and not just a StructTag assumed below.
+        assert!(self.is_struct_tag());
+
+        let mut types: BTreeMap<String, Type> = BTreeMap::new();
+        let fields = symbol_table.lookup_fields_in_type(self).unwrap();
+        for field in fields {
+            if field.is_padding() {
+                continue;
+            }
+            types.insert(field.name().to_string(), field.typ());
+        }
+        types
+    }
+}

compiler/rustc_codegen_llvm/src/gotoc/rvalue.rs

@@ -829,6 +829,15 @@ impl<'tcx> GotocCtx<'tcx> {
             return None; // no cast required, nothing to do
         }
 
+        // The src type will be sized, but the dst type may not be unsized.  If
+        // the dst is an adt containing a pointer to a trait object nested
+        // within the adt, the trait object will be unsized and the pointer will
+        // be a fat pointer, but the adt (containing the fat pointer) will
+        // itself be sized.
+        assert!(
+            src_mir_type.is_sized(self.tcx.at(rustc_span::DUMMY_SP), ty::ParamEnv::reveal_all())
+        );
+
         match (src_mir_type.kind(), dst_mir_type.kind()) {
             (ty::Ref(..), ty::Ref(..)) => {
                 self.cast_sized_pointer_to_fat_pointer(src_goto_expr, src_mir_type, dst_mir_type)
@@ -846,8 +855,10 @@ impl<'tcx> GotocCtx<'tcx> {
                 self.cast_sized_adt_to_unsized_adt(src_goto_expr, src_mir_type, dst_mir_type)
             }
             (src_kind, dst_kind) => {
-                assert!(src_kind == dst_kind);
-                None
+                unreachable!(
+                    "In this case, {:?} and {:?} should have the same type (a case already handled)",
+                    src_kind, dst_kind
+                )
             }
         }
     }
@@ -868,17 +879,17 @@ impl<'tcx> GotocCtx<'tcx> {
 
         // extract pointee types from pointer types, panic if type is not a
         // pointer type.
-        let src_pointee_type = pointee_type(src_mir_type);
-        let dst_pointee_type = pointee_type(dst_mir_type);
+        let src_pointee_type = pointee_type(src_mir_type).unwrap();
+        let dst_pointee_type = pointee_type(dst_mir_type).unwrap();
 
-        let dst_pointee_metadata = dst_pointee_type.ptr_metadata_ty(self.tcx);
+        let dst_pointee_metadata_type = dst_pointee_type.ptr_metadata_ty(self.tcx);
         let unit = self.tcx.types.unit;
         let usize = self.tcx.types.usize;
 
-        if dst_pointee_metadata == unit {
-            assert!(src_pointee_type == dst_pointee_type);
+        if dst_pointee_metadata_type == unit {
+            assert_eq!(src_pointee_type, dst_pointee_type);
             None
-        } else if dst_pointee_metadata == usize {
+        } else if dst_pointee_metadata_type == usize {
             self.cast_sized_pointer_to_slice_fat_pointer(
                 src_goto_expr,
                 src_mir_type,
@@ -887,6 +898,7 @@ impl<'tcx> GotocCtx<'tcx> {
                 dst_pointee_type,
             )
         } else {
+            // dst_pointee_metadata_type == std::ptr::DynMetadata<dyn trait> (a vtable is required)
             self.cast_sized_pointer_to_trait_fat_pointer(
                 src_goto_expr,
                 src_mir_type,
@@ -910,9 +922,9 @@ impl<'tcx> GotocCtx<'tcx> {
     ) -> Option<Expr> {
         match (src_pointee_type.kind(), dst_pointee_type.kind()) {
             (ty::Array(src_elt_type, src_elt_count), ty::Slice(dst_elt_type)) => {
-                assert!(src_elt_type == dst_elt_type);
+                assert_eq!(src_elt_type, dst_elt_type);
                 let dst_goto_type = self.codegen_ty(dst_mir_type);
-                let dst_goto_expr =
+                let dst_goto_expr = // cast from an array type to a pointer type
                     src_goto_expr.cast_to(self.codegen_ty(src_elt_type).to_pointer());
                 let dst_goto_len = self.codegen_const(src_elt_count, None);
                 Some(slice_fat_ptr(dst_goto_type, dst_goto_expr, dst_goto_len, &self.symbol_table))
@@ -940,7 +952,7 @@ impl<'tcx> GotocCtx<'tcx> {
         {
             let dst_goto_type = self.codegen_ty(dst_mir_type);
             let dst_goto_expr = src_goto_expr.cast_to(Type::void_pointer());
-            let vtable = self.codegen_vtable(concrete_type, trait_type).clone();
+            let vtable = self.codegen_vtable(concrete_type, trait_type);
             let vtable_expr = vtable.address_of();
             Some(dynamic_fat_ptr(dst_goto_type, dst_goto_expr, vtable_expr, &self.symbol_table))
         } else {
@@ -974,24 +986,23 @@ impl<'tcx> GotocCtx<'tcx> {
                 src_mir_field_types.get(field).unwrap(),
                 dst_mir_field_types.get(field).unwrap(),
             ) {
-                cast_required.push((field.clone(), expr.clone()));
+                cast_required.push((field.to_string(), expr));
             }
         }
-        // Update the fields for which a cast was required and return the result.
-        let mut update_required = cast_required.len() > 0;
-
-        // A zero-sized type like PhantomData is a struct with no fields, and
-        // hence with no fields for which a cast was required.  But PhantomData
-        // takes a type as an generic parameter, and when casting a sized [u8; 4]
-        // to an unsized [u8], we have to change the type of PhantomData from
+        // Return None for a struct with fields if none of the fields require a cast.
+        //
+        // Note that a struct with no fields may still require a cast.
+        // PhantomData is a zero-sized type that is a struct with no fields, and
+        // hence with no fields that require a cast.  But PhantomData takes a
+        // type as an generic parameter, and when casting a sized [u8; 4] to an
+        // unsized [u8], we have to change the type of PhantomData from
         // PhantomData<[u8; 4]> to PhantomData<[u8]>.
-        update_required = update_required || dst_mir_field_types.is_empty();
-
-        if !update_required {
+        if !dst_mir_field_types.is_empty() && cast_required.is_empty() {
             return None;
         }
 
         for (field, expr) in cast_required {
+            // Replace the field expression with the cast expression
             src_goto_field_values.insert(field.clone(), expr.clone());
         }
         let dst_goto_expr = Expr::struct_expr(
@@ -1002,9 +1013,14 @@ impl<'tcx> GotocCtx<'tcx> {
         Some(dst_goto_expr)
     }
 
-    /// Search for the pair of corresponding concrete and trait types nested
-    /// within a pair of ADTs being cast from a concrete object to a trait
-    /// object.
+    /// Find the trait type and corresponding concrete type in a pair of ADTs.
+    ///
+    /// Given two ADTs with types src and dst, the goal is to cast a thin
+    /// pointer to src to a fat pointer to dst.  Dst has nested within it a
+    /// trait type (a Dynamic).  Src has nested within it at the corresponding
+    /// position a concrete type. This function returns the pair (concrete type,
+    /// trait type) that we can use to build the vtable for the concrete type
+    /// implementation of the trait type.
     fn nested_pair_of_concrete_and_trait_types(
         &self,
         src_mir_type: Ty<'tcx>,
@@ -1017,27 +1033,23 @@ impl<'tcx> GotocCtx<'tcx> {
                 let dst_fields = self.mir_struct_field_types(dst_mir_type);
                 assert!(src_fields.keys().eq(dst_fields.keys()));
 
-                let mut matching_types: Vec<(Ty<'tcx>, Ty<'tcx>)> = vec![];
+                let mut matching_types: Option<(Ty<'tcx>, Ty<'tcx>)> = None;
                 for field in src_fields.keys() {
                     let pair = self.nested_pair_of_concrete_and_trait_types(
                         src_fields.get(field).unwrap(),
                         dst_fields.get(field).unwrap(),
                     );
-                    if let Some((concrete, dynamic)) = pair {
-                        matching_types.push((concrete.clone(), dynamic.clone()))
-                    };
-                }
-                match matching_types.len() {
-                    0 => None,
-                    1 => {
-                        let (concrete, dynamic) = matching_types[0];
-                        Some((concrete, dynamic)) // type checking seems to require this explicit deconstruction
+                    if pair.is_some() {
+                        assert!(
+                            matching_types.is_none(),
+                            "Searching for pairs of concrete and trait types, found multiple pairs in {:?} and {:?}",
+                            src_mir_type,
+                            dst_mir_type
+                        );
+                        matching_types = pair;
                     }
-                    _ => panic!(
-                        "Searching for pairs of concrete and trait types found multiple pairs in {:?} and {:?}",
-                        src_mir_type, dst_mir_type
-                    ),
                 }
+                matching_types
             }
             // In the context of
             //    handling Result::<&i32, ()>::unwrap, std::result::Result::<T, E>::unwrap

compiler/rustc_codegen_llvm/src/gotoc/typ.rs

@@ -992,12 +992,14 @@ impl<'tcx> GotocCtx<'tcx> {
     pub fn mir_struct_field_types(&self, struct_type: Ty<'tcx>) -> BTreeMap<String, Ty<'tcx>> {
         match struct_type.kind() {
             ty::Adt(adt_def, adt_substs) if adt_def.variants.len() == 1 => {
-                let mut types: BTreeMap<String, Ty<'tcx>> = BTreeMap::new();
                 let fields = &adt_def.variants.get(VariantIdx::from_u32(0)).unwrap().fields;
-                for field in fields {
-                    types.insert(field.ident.name.to_string(), field.ty(self.tcx, adt_substs));
-                }
-                types
+                let mut map: BTreeMap<String, Ty<'tcx>> = BTreeMap::new();
+                map.extend(
+                    fields.iter().map(|field| {
+                        (field.ident.name.to_string(), field.ty(self.tcx, adt_substs))
+                    }),
+                );
+                map
             }
             _ => unreachable!("Expected a single-variant ADT. Found {:?}", struct_type),
         }
@@ -1006,10 +1008,10 @@ impl<'tcx> GotocCtx<'tcx> {
 
 /// Extract from a mir pointer type the mir type of the value to which the
 /// pointer points.
-pub fn pointee_type(pointer_type: Ty<'tcx>) -> Ty<'tcx> {
+pub fn pointee_type(pointer_type: Ty<'tcx>) -> Option<Ty<'tcx>> {
     match pointer_type.kind() {
-        ty::Ref(_, pointee_type, _) => pointee_type,
-        ty::RawPtr(ty::TypeAndMut { ty: pointee_type, .. }) => pointee_type,
-        _ => panic!("Expected a pointer type."),
+        ty::Ref(_, pointee_type, _) => Some(pointee_type),
+        ty::RawPtr(ty::TypeAndMut { ty: pointee_type, .. }) => Some(pointee_type),
+        _ => None,
     }
 }

rust-tests/cbmc-reg/Closure/main.rs

@@ -2,6 +2,10 @@
 // SPDX-License-Identifier: Apache-2.0 OR MIT
 //! test that we implement closures correctly
 
+// Commenting out the test that induces the issue described in 
+// https://github.com/model-checking/rmc/issues/83
+// until this issue is resolved.
+
 /*
 fn closure_with_empty_args() {
     let bytes = vec![0];
@@ -41,9 +45,7 @@ fn test_env() {
 }
 
 fn main() {
-    /*
-        closure_with_empty_args();
-    */
+    // closure_with_empty_args();
     closure_with_1_arg();
     test_three_args();
     test_unit_args();