Track whether padding should be forced

Partially handles rust-diplomat#657

tool/src/js/layout.rs

@@ -18,6 +18,8 @@ pub struct StructFieldLayout {
     pub padding_count: usize,
     /// The size of the padding field
     pub padding_size: usize,
+    /// The number of scalar fields in this field
+    pub scalar_count: usize,
 }
 
 pub struct StructFieldsInfo {
@@ -67,6 +69,7 @@ pub fn struct_field_info<'a, P: hir::TyPosition + 'a>(
             offset: next_offset,
             padding_count: padding / padding_size,
             padding_size,
+            scalar_count: field_scalars,
         });
         // All fields use padding sizes from the *previous*
         padding_size = align;

tool/src/js/mod.rs

@@ -138,16 +138,29 @@ pub(crate) fn run<'tcx>(
                 TypeDef::Enum(e) => context.gen_enum(ts, &name, e, &methods_info),
                 TypeDef::Opaque(o) => context.gen_opaque(ts, &name, o, &methods_info),
                 TypeDef::Struct(s) => {
-                    context.gen_struct(ts, &name, s, &fields.clone().unwrap(), &methods_info, false)
+                    let (fields, needs_force_padding) = fields.clone().unwrap();
+                    context.gen_struct(
+                        ts,
+                        &name,
+                        s,
+                        &fields,
+                        &methods_info,
+                        false,
+                        needs_force_padding,
+                    )
+                }
+                TypeDef::OutStruct(s) => {
+                    let (fields, needs_force_padding) = fields_out.clone().unwrap();
+                    context.gen_struct(
+                        ts,
+                        &name,
+                        s,
+                        &fields,
+                        &methods_info,
+                        true,
+                        needs_force_padding,
+                    )
                 }
-                TypeDef::OutStruct(s) => context.gen_struct(
-                    ts,
-                    &name,
-                    s,
-                    &fields_out.clone().unwrap(),
-                    &methods_info,
-                    true,
-                ),
                 _ => unreachable!("HIR/AST variant {:?} is unknown.", type_def),
             };
 

tool/src/js/type_generation/converter.rs

@@ -19,6 +19,21 @@ fn is_contiguous_enum(ty: &hir::EnumDef) -> bool {
         .all(|(i, v)| i as isize == v.discriminant)
 }
 
+/// The Rust-Wasm ABI currently treats structs with 1 or 2 scalar fields different from
+/// structs with more ("large" structs). Structs with 1 or 2 scalar fields are passed in as consecutive fields,
+/// whereas larger structs are passed in as an array of fields *including padding*. This choice is typically at the struct
+/// level, however a small struct found within a large struct will also need to care about padding.
+#[derive(Copy, Clone, Default, PartialEq, Eq)]
+pub(super) enum ForcePaddingStatus {
+    /// Don't force padding. For structs found in arguments
+    #[default]
+    NoForce,
+    /// Force padding. For small structs found as fields in large structs
+    Force,
+    /// Force padding if the caller forces padding. For small structs found as fields in small structs.
+    PassThrough,
+}
+
 /// Context about a struct being borrowed when doing js-to-c conversions
 /// Borrowed from dart implementation.
 pub(super) struct StructBorrowContext<'tcx> {
@@ -552,14 +567,19 @@ impl<'jsctx, 'tcx> TyGenContext<'jsctx, 'tcx> {
         js_name: Cow<'tcx, str>,
         struct_borrow_info: Option<&StructBorrowContext<'tcx>>,
         alloc: Option<&str>,
+        // Whether to force padding
+        force_padding: ForcePaddingStatus,
     ) -> Cow<'tcx, str> {
         match *ty {
             Type::Primitive(..) => js_name.clone(),
             Type::Opaque(ref op) if op.is_optional() => format!("{js_name}.ffiValue ?? 0").into(),
             Type::Enum(..) | Type::Opaque(..) => format!("{js_name}.ffiValue").into(),
-            Type::Struct(..) => {
-                self.gen_js_to_c_for_struct_type(js_name, struct_borrow_info, alloc.unwrap())
-            }
+            Type::Struct(..) => self.gen_js_to_c_for_struct_type(
+                js_name,
+                struct_borrow_info,
+                alloc.unwrap(),
+                force_padding,
+            ),
             Type::Slice(slice) => {
                 if let Some(hir::MaybeStatic::Static) = slice.lifetime() {
                     panic!("'static not supported for JS backend.")
@@ -605,6 +625,7 @@ impl<'jsctx, 'tcx> TyGenContext<'jsctx, 'tcx> {
         js_name: Cow<'tcx, str>,
         struct_borrow_info: Option<&StructBorrowContext<'tcx>>,
         allocator: &str,
+        force_padding: ForcePaddingStatus,
     ) -> Cow<'tcx, str> {
         let mut params = String::new();
         if let Some(info) = struct_borrow_info {
@@ -629,7 +650,12 @@ impl<'jsctx, 'tcx> TyGenContext<'jsctx, 'tcx> {
                 write!(&mut params, "],").unwrap();
             }
         }
-        format!("...{js_name}._intoFFI({allocator}, {{{params}}})").into()
+        let force_padding = match force_padding {
+            ForcePaddingStatus::NoForce => "",
+            ForcePaddingStatus::Force => ", true",
+            ForcePaddingStatus::PassThrough => ", forcePadding",
+        };
+        format!("...{js_name}._intoFFI({allocator}, {{{params}}}{force_padding})").into()
     }
     // #endregion
 }

tool/src/js/type_generation/mod.rs

@@ -20,7 +20,7 @@ use super::formatter::JSFormatter;
 use crate::ErrorStore;
 
 mod converter;
-use converter::StructBorrowContext;
+use converter::{ForcePaddingStatus, StructBorrowContext};
 
 /// Represents context for generating a Javascript class.
 ///
@@ -155,16 +155,21 @@ impl<'ctx, 'tcx> TyGenContext<'ctx, 'tcx> {
         .unwrap()
     }
 
-    /// Generate a list of [`FieldInfo`] to be used in [`Self::gen_struct`]. We separate this step out for two reasons:
+    /// Generate a list of [`FieldInfo`] to be used in [`Self::gen_struct`].
+    ///
+    /// Also returns a boolean of whether the forcePadding argument is needed.
+    ///
+    /// We separate this step out for two reasons:
     ///
     /// 1. It allows re-use between `.d.ts` and `.mjs` files.
     /// 2. Clarity.
     pub(super) fn generate_fields<P: hir::TyPosition>(
         &self,
         struct_def: &'tcx hir::StructDef<P>,
-    ) -> Vec<FieldInfo<P>> {
+    ) -> (Vec<FieldInfo<P>>, bool) {
         let struct_field_info =
             crate::js::layout::struct_field_info(struct_def.fields.iter().map(|f| &f.ty), self.tcx);
+        let mut needs_force_padding = false;
 
         let fields = struct_def.fields.iter().enumerate()
         .map(|(i, field)| {
@@ -215,29 +220,54 @@ impl<'ctx, 'tcx> TyGenContext<'ctx, 'tcx> {
 
             let padding = struct_field_info.fields[i].padding_count;
 
+
+            let force_padding = match (struct_field_info.fields[i].scalar_count, struct_field_info.scalar_count) {
+                // There's no padding needed
+                (0 | 1, _) => ForcePaddingStatus::NoForce,
+                // Non-structs don't care
+                _ if !matches!(&field.ty, &hir::Type::Struct(_)) => ForcePaddingStatus::NoForce,
+                // 2-field struct contained in 2-field struct, caller decides
+                (2, 2) => {
+                    needs_force_padding = true;
+                    ForcePaddingStatus::PassThrough
+                }
+                // Outer struct has > 3 fields, always pad
+                (2, 3..) => ForcePaddingStatus::Force,
+                // Larger fields will always have padding anyway
+                _ => ForcePaddingStatus::NoForce
+
+            };
+
             let maybe_preceding_padding = if padding > 0 {
                 let padding_size_str = match struct_field_info.fields[i].padding_size {
-                    1 => "u8",
-                    2 => "u16",
-                    4 => "u32",
-                    8 => "u64",
+                    1 => "i8",
+                    2 => "i16",
+                    4 => "i32",
+                    8 => "i64",
                     other => unreachable!("Found unknown padding size {other}")
                 };
-                let mut out = format!("/* Padding ({padding_size_str}) for {} */ ", field.name);
 
-                for i in 0..padding {
-                    if i < padding - 1 {
-                        write!(out, "0, ").unwrap();
-                    } else {
-                        write!(out, "0 /* End Padding */,").unwrap();
+                if struct_field_info.scalar_count == 2 {
+                    // For structs with 2 scalar fields, we pass down whether or not padding is needed from the caller
+                    needs_force_padding = true;
+                    format!("...diplomatRuntime.maybePaddingFields(forcePadding, {padding} /* x {padding_size_str} */), ")
+                } else {
+                    let mut out = format!("/* [{padding} x {padding_size_str}] padding */ ");
+                    for i in 0..padding {
+                        if i < padding - 1 {
+                            write!(out, "0, ").unwrap();
+                        } else {
+                            write!(out, "0 /* end padding */, ").unwrap();
+                        }
                     }
+
+                    out
                 }
 
-                out
             } else {
                 "".into()
             };
-            let js_to_c = self.gen_js_to_c_for_type(&field.ty, format!("this.#{}", field_name.clone()).into(), maybe_struct_borrow_info.as_ref(), alloc.as_deref());
+            let js_to_c = self.gen_js_to_c_for_type(&field.ty, format!("this.#{}", field_name.clone()).into(), maybe_struct_borrow_info.as_ref(), alloc.as_deref(), force_padding);
             let js_to_c = format!("{maybe_preceding_padding}{js_to_c}");
 
             FieldInfo {
@@ -250,7 +280,8 @@ impl<'ctx, 'tcx> TyGenContext<'ctx, 'tcx> {
                 maybe_struct_borrow_info: maybe_struct_borrow_info.map(|i| i.param_info)
             }
         }).collect::<Vec<_>>();
-        fields
+
+        (fields, needs_force_padding)
     }
 
     /// Generate a struct type's body for a file from the given definition.
@@ -267,6 +298,7 @@ impl<'ctx, 'tcx> TyGenContext<'ctx, 'tcx> {
         methods: &MethodsInfo,
 
         is_out: bool,
+        needs_force_padding: bool,
     ) -> String {
         #[derive(Template)]
         #[template(path = "js/struct.js.jinja", escape = "none")]
@@ -276,6 +308,7 @@ impl<'ctx, 'tcx> TyGenContext<'ctx, 'tcx> {
             typescript: bool,
             mutable: bool,
             is_out: bool,
+            needs_force_padding: bool,
 
             lifetimes: &'a LifetimeEnv,
             fields: &'a Vec<FieldInfo<'a, P>>,
@@ -290,6 +323,7 @@ impl<'ctx, 'tcx> TyGenContext<'ctx, 'tcx> {
             typescript,
             is_out,
             mutable: !is_out,
+            needs_force_padding,
 
             lifetimes: &struct_def.lifetimes,
             fields,
@@ -372,7 +406,9 @@ impl<'ctx, 'tcx> TyGenContext<'ctx, 'tcx> {
                                 "Slices must produce slice ParamBorrowInfo, found {param_borrow_kind:?}"
                             ),
                         }
-                    ))
+                    ),
+                    // Arguments never force padding
+                    ForcePaddingStatus::NoForce)
                 );
 
                 // We add the pointer and size for slices:
@@ -409,6 +445,8 @@ impl<'ctx, 'tcx> TyGenContext<'ctx, 'tcx> {
                         param_info.name.clone(),
                         struct_borrow_info.as_ref(),
                         alloc,
+                        // Arguments never force padding
+                        ForcePaddingStatus::NoForce,
                     ));
             }
 

tool/templates/js/runtime.mjs

@@ -59,6 +59,18 @@ export function enumDiscriminant(wasm, ptr) {
     return (new Int32Array(wasm.memory.buffer, ptr, 1))[0]
 }
 
+/**
+ * Return an array of paddingCount zeroes to be spread into a function call
+ * if needsPaddingFields is true, else empty
+ */
+export function maybePaddingFields(needsPaddingFields, paddingCount) {
+    if (needsPaddingFields) {
+        return Array(paddingCount).fill(0);;
+    } else {
+        return [];
+    }
+}
+
 /** 
  * A wrapper around a slice of WASM memory that can be freed manually or
  * automatically by the garbage collector.

tool/templates/js/struct.js.jinja

@@ -61,10 +61,17 @@ export class {{type_name}} {
     //
     // This method does not handle lifetime relationships: if `'foo: 'bar`, make sure fooAppendArray contains everything barAppendArray does.
     {%- endif -%}
-    {% endif %}
+    {%- endif %}
+    {%- if needs_force_padding %}
+    // JS structs need to be generated with or without padding depending on whether they are being passed as aggregates or splatted out into fields.
+    // Most of the time this is known beforehand: large structs (>2 scalar fields) always get padding, and structs passed directly in parameters omit padding
+    // if they are small. However small structs within large structs also get padding, and we signal that by setting forcePadding.
+    {%- endif %}
     _intoFFI(
         functionCleanupArena,
-        appendArrayMap
+        appendArrayMap{% if needs_force_padding %},
+        forcePadding
+        {%- endif %}
     ) {
         return [
             {%- for field in fields -%}