Repository: mitsuhiko/minijinja (language: rust)

Issue:
Update minijinja-cabi to avoid depending on usize bit length
Hi! 

While trying to build minijinja-cabi for Wasm targets, I encountered an error due to a size mismatch on transmute operations, the reason is that `usize` is 32 bits on those targets which breaks existing assumption for usize to be 64 bits, this attempts to fix this by doing the following:

* Add `const VALUE_SIZE: usize = size_of::<Value>();`
* Change `_opaque: [usize; 3],` to `_opaque: [u8; VALUE_SIZE],` in the `mj_value` struct.
* Change `transmute::<Value, [usize; 3]>(value)` to `transmute::<Value, [u8; VALUE_SIZE]>(value)` in the `impl From<Value> for mj_value` block.

Does this look like the correct approach?

For reference here is the build error without those changes when running `cargo build -p minijinja-cabi --target wasm32-wasi`

> Compiling minijinja-cabi v2.3.1 (/workspaces/minijinja/minijinja-cabi)
> error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
>   --> minijinja-cabi/src/value.rs:47:18
>    |
> 47 |         unsafe { transmute(self._opaque) }
>    |                  ^^^^^^^^^
>    |
>    = note: source type: `[usize; 3]` (96 bits)
>    = note: target type: `Value` (192 bits)
> 
> error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
>   --> minijinja-cabi/src/value.rs:54:31
>    |
> 54 |             _opaque: unsafe { transmute::<Value, [usize; 3]>(value) },
>    |                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>    |
>    = note: source type: `Value` (192 bits)
>    = note: target type: `[usize; 3]` (96 bits)
> 
> error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
>    --> minijinja-cabi/src/value.rs:380:46
>     |
> 380 |         let mut value: ManuallyDrop<Value> = transmute((*value)._opaque);
>     |                                              ^^^^^^^^^
>     |
>     = note: source type: `[usize; 3]` (96 bits)
>     = note: target type: `ManuallyDrop<Value>` (192 bits)
> 
> For more information about this error, try `rustc --explain E0512`.
> warning: `minijinja-cabi` (lib) generated 1 warning (1 duplicate)
> error: could not compile `minijinja-cabi` (lib) due to 3 previous errors; 1 warning emitted


Discussion / hints:
The problem I see is that `u8` has absolutely no alignment requirements.  I'm not entirely sure what the best solution is.
@mitsuhiko From what I understand using `align(8)`:
```rust
#[repr(C, align(8))]
pub struct mj_value {
    _opaque: [u8; VALUE_SIZE],
}
```

Would probably offer the proper alignement guarantees as long as `ValueRepr` never stops using `Packed` 128 bit integers so that it stays 8 bytes aligned, however I might be missing some details.
What do you think?
EDIT:
Unfortunately the example below is considered as non FFI safe so I suppose #[repr(C, align(8))] might be the best way, probably also adding a test and documentation enabling to guarantee `Value` and `mj_value` keep the same alignement in the future, see the latest changes.

---

I see another option in [this answer to a Stack Overflow question](https://stackoverflow.com/a/75827357/2801589) which suggests using a `[T, 0]` field to align a parent structure to the same value as `T`, which adapted to our case would probably look something like this:
```rust
#[repr(C)]
pub struct mj_value {
    _align: [Value; 0],
    _opaque: [u8; VALUE_SIZE]
}
```

The post does note that even then there are subtleties when dealing with FFI, however I wonder if this is actually an issue as long as the type is treated as an opaque value by the C code?

Am I correct that as long as all these conditions are met:
- C treats `mj_value` as opaque
- `mj_value` has the same size as `Value`
- `mj_value` has the same alignement as `Value`

Then we can safely transmute between `Value` and `mj_value`? If so it would seem like this solution is a good one.


What does this even generate in C?  Can you run bindgen to see what it spits out?  I'm assuming it is not generating valid C.

```rust
#[repr(C)]
pub struct mj_value {
    _align: [Value; 0],
    _opaque: [u8; VALUE_SIZE]
}
```
I cannot actually get `cbindgen` to correctly use a constant there.  This is related to this issue: https://github.com/mozilla/cbindgen/issues/786


See also https://github.com/mozilla/cbindgen/issues/593

Fix patch:
diff --git a/minijinja-cabi/include/minijinja.h b/minijinja-cabi/include/minijinja.h
index 348ea95c..f7f381aa 100644
--- a/minijinja-cabi/include/minijinja.h
+++ b/minijinja-cabi/include/minijinja.h
@@ -92,7 +92,7 @@ typedef struct mj_value_iter mj_value_iter;
  Opaque value type.
  */
 typedef struct mj_value {
-  uintptr_t _opaque[3];
+  uint64_t _opaque[3];
 } mj_value;
 
 /*
@@ -414,7 +414,7 @@ MINIJINJA_API char *mj_value_to_str(struct mj_value value);
 MINIJINJA_API struct mj_value_iter *mj_value_try_iter(struct mj_value value);
 
 #ifdef __cplusplus
-}  // extern "C"
-#endif  // __cplusplus
+} // extern "C"
+#endif // __cplusplus
 
-#endif  /* _minijinja_h_included */
+#endif /* _minijinja_h_included */
diff --git a/minijinja-cabi/src/value.rs b/minijinja-cabi/src/value.rs
index d5a2a525..3738e931 100644
--- a/minijinja-cabi/src/value.rs
+++ b/minijinja-cabi/src/value.rs
@@ -39,7 +39,15 @@ where
 /// Opaque value type.
 #[repr(C)]
 pub struct mj_value {
-    _opaque: [usize; 3],
+    // Motivation on the size here: The size of `Value` is really not
+    // known and since cbindgen has no way to guarnatee us a matching
+    // size we have to be creative.  The dominating type size wise is
+    // most likely going to be SmallStr which is a u8+[u8; 22] plus the
+    // enum discriminant (u8).
+    //
+    // We are going with u64 here for alignment reasons which is likely
+    // to be a good default across platforms.
+    _opaque: [u64; 3],
 }
 
 impl mj_value {
@@ -51,7 +59,7 @@ impl mj_value {
 impl From<Value> for mj_value {
     fn from(value: Value) -> Self {
         mj_value {
-            _opaque: unsafe { transmute::<Value, [usize; 3]>(value) },
+            _opaque: unsafe { transmute::<Value, [u64; 3]>(value) },
         }
     }
 }


Test patch: