Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update minijinja-cabi to avoid depending on usize bit length #601

Closed
wants to merge 3 commits into from

Conversation

PopFlamingo
Copy link
Contributor

@PopFlamingo PopFlamingo commented Oct 21, 2024

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 = 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

Hi! 

While trying to build minijinja-cabi for Wasm targets, I encountered an error due to a size mismatch on transmute operations, due to `usize` being 32 bits on those targets which breaks existing size assumptions, 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?
@PopFlamingo PopFlamingo changed the title Update minijinja cabi to account for usize changes Update minijinja-cabi to account for usize changes Oct 21, 2024
@PopFlamingo PopFlamingo changed the title Update minijinja-cabi to account for usize changes Update minijinja-cabi to avoid depending on usize bit length Oct 21, 2024
@mitsuhiko
Copy link
Owner

The problem I see is that u8 has absolutely no alignment requirements. I'm not entirely sure what the best solution is.

@PopFlamingo
Copy link
Contributor Author

PopFlamingo commented Oct 21, 2024

@mitsuhiko From what I understand using align(8):

#[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?

@PopFlamingo
Copy link
Contributor Author

PopFlamingo commented Oct 21, 2024

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 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:

#[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.

This also adds tests to check for matching size and alignement
@mitsuhiko
Copy link
Owner

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.

#[repr(C)]
pub struct mj_value {
    _align: [Value; 0],
    _opaque: [u8; VALUE_SIZE]
}

@mitsuhiko
Copy link
Owner

mitsuhiko commented Oct 22, 2024

I cannot actually get cbindgen to correctly use a constant there. This is related to this issue: mozilla/cbindgen#786

See also mozilla/cbindgen#593

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants