RTS: remove repr(packed) attributes from RTS structs (#2764)

This is to remove some of the potential undefined behaviors (UB). It will also
remove some of the syntactic noise in #2761.

Rust has alignment restrictions for types and fields beyond the hardware
limitations. This means even on Wasm (which has no alignment restrictions) we
have some restrictions to deal with.

Recently the compiler started to check for some of the obvious sources of UB
(rust-lang/rust#82523). One of these is when taking a
pointer or reference to a `packed` struct. Since `packed` means no padding
between fields, the fields may be unaligned, in which case getting a reference
to them would be UB.

To fix this, this PR removes `packed` attributes and refactors the `Bits64`
type to make sure that the layout is as before. It turns out for types other
than `Bits64` we don't need `packed`: all fields are word sizes so the compiler
does not add any padding. For `Bits64`, we had a `u64` field which is aligned
on 64-bit boundary. To avoid this we now split 64-bit payload into two 32-bit
fields.

A new module `static_checks` added to make sure struct sizes are as expected.
Assertions in this module are checked in compile time.

No extra work needed to align objects. All objects need 4 bytes alignment
(checked with `core::mem::align_of`). The compiler already [aligns static
objects][2], and in runtime we only allocate whole words. So both static and
dynamic objects are always aligned. Debug mode assertions added in GC to check
object alignment.

[1]: https://doc.rust-lang.org/stable/reference/
[2]: https://github.com/dfinity/motoko/blob/59ddaa4520793b6e7038b60e3c30ff267d8415f6/src/codegen/compile.ml#L453

default.nix

@@ -159,7 +159,7 @@ rec {
         name = "motoko-rts-deps";
         src = subpath ./rts;
         sourceRoot = "rts/motoko-rts-tests";
-        sha256 = "0sy7jglz9pxw2lz0qjyplchcfn78d7789sd93xwybisamjynlniy";
+        sha256 = "0jyp3j8n5bj5cy1fd26d7h55zmc4v14qc2w8adxqwmsv5riqz41g";
         copyLockfile = true;
       };
     in

rts/motoko-rts/Cargo.toml

@@ -21,6 +21,7 @@ ic = []
 [dependencies]
 libc = { version = "0.2.81", default_features = false }
 motoko-rts-macros = { path = "../motoko-rts-macros" }
+static_assertions = "1.1.0"
 
 # Added here so that it ends up in Cargo.lock, so that nix will pre-fetch it
 [dependencies.compiler_builtins]

rts/motoko-rts/native/Cargo.toml

@@ -11,6 +11,7 @@ path = "../src/lib.rs"
 [dependencies]
 libc = { version = "0.2.73", default_features = false }
 motoko-rts-macros = { path = "../../motoko-rts-macros" }
+static_assertions = "1.1.0"
 
 [dependencies.compiler_builtins]
 version = "0.1.39"

rts/motoko-rts/src/debug.rs

@@ -175,7 +175,7 @@ pub(crate) unsafe fn print_boxed_object(buf: &mut WriteBuf, p: usize) {
         }
         TAG_BITS64 => {
             let bits64 = obj as *const Bits64;
-            let _ = write!(buf, "<Bits64 {:#x}>", (*bits64).bits);
+            let _ = write!(buf, "<Bits64 {:#x}>", (*bits64).bits());
         }
         TAG_MUTBOX => {
             let mutbox = obj as *const MutBox;

rts/motoko-rts/src/gc/copying.rs

@@ -1,3 +1,4 @@
+use crate::constants::WORD_SIZE;
 use crate::mem_utils::{memcpy_bytes, memcpy_words};
 use crate::memory::Memory;
 use crate::types::*;
@@ -125,6 +126,9 @@ unsafe fn evac<M: Memory>(
 
     let obj = (*ptr_loc).as_obj();
 
+    // Check object alignment to avoid undefined behavior. See also static_checks module.
+    debug_assert_eq!(obj as u32 % WORD_SIZE, 0);
+
     // Update the field if the object is already evacauted
     if obj.tag() == TAG_FWD_PTR {
         let fwd = (*(obj as *const FwdPtr)).fwd;

rts/motoko-rts/src/gc/mark_compact.rs

@@ -127,6 +127,9 @@ unsafe fn mark_object<M: Memory>(mem: &mut M, obj: Value, heap_base: u32) {
     let obj_tag = obj.tag();
     let obj = obj.get_ptr() as u32;
 
+    // Check object alignment to avoid undefined behavior. See also static_checks module.
+    debug_assert_eq!(obj % WORD_SIZE, 0);
+
     let obj_idx = (obj - heap_base) / WORD_SIZE;
 
     if get_bit(obj_idx) {

rts/motoko-rts/src/lib.rs

@@ -32,6 +32,7 @@ pub mod leb128;
 mod mem_utils;
 pub mod memory;
 pub mod principal_id;
+mod static_checks;
 pub mod text;
 pub mod text_iter;
 mod tommath_bindings;

rts/motoko-rts/src/static_checks.rs

@@ -0,0 +1,54 @@
+//! Static assertions to make sure object layouts are as expected
+
+use crate::types::*;
+
+use core::mem::{align_of, size_of};
+
+use static_assertions::const_assert_eq;
+
+// TODO: I don't understand why I get a "unused constant" warning for this. Removing it causes
+// compilation failures as expected.
+#[allow(unused)]
+const WORD_SIZE: usize = crate::constants::WORD_SIZE as usize;
+
+// Check platform word size
+const_assert_eq!(size_of::<usize>(), size_of::<u32>());
+const_assert_eq!(size_of::<usize>(), WORD_SIZE);
+
+// Check that sizes of structs are as expected by the compiler
+// (Expectations are all over the place, e.g. `header_size` definitions in `compile.ml`, calls to `static_closure`, etc.)
+const_assert_eq!(size_of::<Obj>(), 1 * WORD_SIZE);
+const_assert_eq!(size_of::<ObjInd>(), 2 * WORD_SIZE);
+const_assert_eq!(size_of::<Closure>(), 3 * WORD_SIZE);
+const_assert_eq!(size_of::<Blob>(), 2 * WORD_SIZE);
+const_assert_eq!(size_of::<BigInt>(), 5 * WORD_SIZE);
+const_assert_eq!(size_of::<MutBox>(), 2 * WORD_SIZE);
+const_assert_eq!(size_of::<Some>(), 2 * WORD_SIZE);
+const_assert_eq!(size_of::<Variant>(), 3 * WORD_SIZE);
+const_assert_eq!(size_of::<Concat>(), 4 * WORD_SIZE);
+const_assert_eq!(size_of::<Null>(), 1 * WORD_SIZE);
+const_assert_eq!(size_of::<Bits32>(), 2 * WORD_SIZE);
+const_assert_eq!(size_of::<Bits64>(), 3 * WORD_SIZE);
+
+// These aren't used generated by the compiler
+const_assert_eq!(size_of::<OneWordFiller>(), 1 * WORD_SIZE);
+const_assert_eq!(size_of::<FreeSpace>(), 2 * WORD_SIZE);
+const_assert_eq!(size_of::<FwdPtr>(), 2 * WORD_SIZE);
+
+// Check that objects need to be aligned on word boundaries. Having a different alignment
+// restriction an object type would require changing allocation routines for it.
+const_assert_eq!(align_of::<Obj>(), WORD_SIZE);
+const_assert_eq!(align_of::<ObjInd>(), WORD_SIZE);
+const_assert_eq!(align_of::<Closure>(), WORD_SIZE);
+const_assert_eq!(align_of::<Blob>(), WORD_SIZE);
+const_assert_eq!(align_of::<BigInt>(), WORD_SIZE);
+const_assert_eq!(align_of::<MutBox>(), WORD_SIZE);
+const_assert_eq!(align_of::<Some>(), WORD_SIZE);
+const_assert_eq!(align_of::<Variant>(), WORD_SIZE);
+const_assert_eq!(align_of::<Concat>(), WORD_SIZE);
+const_assert_eq!(align_of::<Null>(), WORD_SIZE);
+const_assert_eq!(align_of::<Bits32>(), WORD_SIZE);
+const_assert_eq!(align_of::<Bits64>(), WORD_SIZE);
+const_assert_eq!(align_of::<OneWordFiller>(), WORD_SIZE);
+const_assert_eq!(align_of::<FreeSpace>(), WORD_SIZE);
+const_assert_eq!(align_of::<FwdPtr>(), WORD_SIZE);

rts/motoko-rts/src/types.rs

@@ -309,7 +309,6 @@ pub const TAG_ONE_WORD_FILLER: Tag = 16;
 pub const TAG_FREE_SPACE: Tag = 17;
 
 // Common parts of any object. Other object pointers can be coerced into a pointer to this.
-#[repr(packed)]
 pub struct Obj {
     pub tag: Tag,
 }
@@ -330,7 +329,6 @@ impl Obj {
     }
 }
 
-#[repr(packed)]
 #[rustfmt::skip]
 pub struct Array {
     pub header: Obj,
@@ -363,7 +361,6 @@ impl Array {
     }
 }
 
-#[repr(packed)]
 pub struct Object {
     pub header: Obj,
     pub size: u32,     // Number of elements
@@ -385,13 +382,11 @@ impl Object {
     }
 }
 
-#[repr(packed)]
 pub struct ObjInd {
     pub header: Obj,
     pub field: Value,
 }
 
-#[repr(packed)]
 pub struct Closure {
     pub header: Obj,
     pub funid: u32,
@@ -409,7 +404,6 @@ impl Closure {
     }
 }
 
-#[repr(packed)]
 pub struct Blob {
     pub header: Obj,
     pub len: Bytes<u32>,
@@ -458,13 +452,11 @@ impl Blob {
 }
 
 /// A forwarding pointer placed by the GC in place of an evacuated object.
-#[repr(packed)]
 pub struct FwdPtr {
     pub header: Obj,
     pub fwd: Value,
 }
 
-#[repr(packed)]
 pub struct BigInt {
     pub header: Obj,
     /// The data following now must describe is the `mp_int` struct.
@@ -503,26 +495,22 @@ impl BigInt {
     }
 }
 
-#[repr(packed)]
 pub struct MutBox {
     pub header: Obj,
     pub field: Value,
 }
 
-#[repr(packed)]
 pub struct Some {
     pub header: Obj,
     pub field: Value,
 }
 
-#[repr(packed)]
 pub struct Variant {
     pub header: Obj,
     pub tag: u32,
     pub field: Value,
 }
 
-#[repr(packed)]
 pub struct Concat {
     pub header: Obj,
     pub n_bytes: Bytes<u32>,
@@ -540,31 +528,35 @@ impl Concat {
     }
 }
 
-#[repr(packed)]
 pub struct Null {
     pub header: Obj,
 }
 
-#[repr(packed)]
 pub struct Bits64 {
     pub header: Obj,
-    pub bits: u64,
+    // We have two 32-bit fields instead of one 64-bit to avoid aligning the fields on 64-bit
+    // boundary.
+    bits_lo: u32,
+    bits_hi: u32,
+}
+
+impl Bits64 {
+    pub fn bits(&self) -> u64 {
+        (u64::from(self.bits_hi) << 32) | u64::from(self.bits_lo)
+    }
 }
 
-#[repr(packed)]
 pub struct Bits32 {
     pub header: Obj,
     pub bits: u32,
 }
 
 /// Marks one word empty space in heap
-#[repr(packed)]
 pub struct OneWordFiller {
     pub header: Obj,
 }
 
 /// Marks arbitrary sized emtpy space in heap
-#[repr(packed)]
 pub struct FreeSpace {
     pub header: Obj,
     pub words: Words<u32>,