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

Investigate why CI miri jobs are failing recently #1200

Closed
Robbepop opened this issue Sep 25, 2024 · 2 comments · Fixed by #1201
Closed

Investigate why CI miri jobs are failing recently #1200

Robbepop opened this issue Sep 25, 2024 · 2 comments · Fixed by #1201

Comments

@Robbepop
Copy link
Member

Robbepop commented Sep 25, 2024

The miri CI jobs are failing roughly since this PR: (2024-09-13)
#1173 (comment)

CI logs stating the failure: https://github.com/wasmi-labs/wasmi/actions/runs/10852181007/job/30117620745

The PR merged before this linked PR and where the miri CI jobs still worked was this: #1172 (2024-09-05)

Commits on the miri repository between 2024-09-05 and 2024-09-13 are:
https://github.com/rust-lang/miri/commits/master/?since=2024-09-05&until=2024-09-12
Unfortunately due to roll-ups it is more difficult to disentangle the changes to miri.

The problem is with this abtraction to (unsafely) convert between Wasmi reference types and UntypedVal:
https://github.com/wasmi-labs/wasmi/blob/main/crates/wasmi/src/reftype.rs
And in particular its usage here: https://github.com/wasmi-labs/wasmi/blob/main/crates/wasmi/src/func/funcref.rs#L55

What the code in question does is a fancy transmute as stated by the Rust reference:
https://doc.rust-lang.org/reference/items/unions.html#reading-and-writing-union-fields
The only missing part is that the Transposer union is missing a #[repr(C)] attribute which we should add. Local tests confirmed though that adding #[repr(C)] does not fix the miri warning.

This PR makes the implicit transmute more explicit and adds the missing #[repr(C)], though miri is still unhappy about it:
https://github.com/wasmi-labs/wasmi/tree/rf-fix-miri-errors

@Robbepop
Copy link
Member Author

Robbepop commented Sep 26, 2024

I crafted a minimal test case that pretty much does the same as Wasmi does:

use core::mem;
use core::num::NonZeroU32;

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
#[repr(transparent)]
pub struct FuncRef {
    funcref: Option<Func>,
}

impl FuncRef {
    pub fn null() -> Self {
        Self { funcref: None }
    }

    pub fn is_null(self) -> bool {
        self.funcref.is_none()
    }

    fn canonicalize(self) -> Self {
        if self.is_null() {
            // Safety: This is safe since `0u64` can be bit
            //         interpreted as a valid `FuncRef` value.
            return u64_to_funcref(0_u64)
        }
        self
    }
}

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
#[repr(C)]
pub struct Func {
    store: u32,
    index: NonZeroU32,
}

fn funcref_to_u64(funcref: FuncRef) -> u64 {
    unsafe { mem::transmute::<FuncRef, u64>(funcref.canonicalize()) }
}

fn u64_to_funcref(value: u64) -> FuncRef {
    unsafe { mem::transmute::<u64, FuncRef>(value) }.canonicalize()
}

#[test]
fn test_null_to_zero() {
    assert_eq!(funcref_to_u64(FuncRef::null()), 0_u64);
    assert!(u64_to_funcref(0_u64).is_null());
}

#[test]
fn test_funcref_to_u64() {
    let funcref = FuncRef { funcref: Some(Func { store: 1, index: NonZeroU32::new(2).unwrap() }) };
    assert_eq!(funcref, u64_to_funcref(funcref_to_u64(funcref)));
}

@Robbepop
Copy link
Member Author

Based on the above minimal test case I was able to identify the location of the bug: It is in the canonicalize function. If we shift the work out of it we get the following piece of code that now is accepted by miri.

use core::mem;
use core::num::NonZeroU32;

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
#[repr(transparent)]
pub struct FuncRef {
    funcref: Option<Func>,
}

impl FuncRef {
    pub fn null() -> Self {
        Self { funcref: None }
    }

    pub fn is_null(self) -> bool {
        self.funcref.is_none()
    }
}

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
#[repr(C)]
pub struct Func {
    store: u32,
    index: NonZeroU32,
}

fn funcref_to_u64(funcref: FuncRef) -> u64 {
    if funcref.is_null() {
        return 0_u64
    }
    unsafe { mem::transmute::<FuncRef, u64>(funcref) }
}

fn u64_to_funcref(value: u64) -> FuncRef {
    if value == 0 {
        return FuncRef::null()
    }
    unsafe { mem::transmute::<u64, FuncRef>(value) }
}

#[test]
fn test_null_to_zero() {
    assert_eq!(funcref_to_u64(FuncRef::null()), 0_u64);
    assert!(u64_to_funcref(0_u64).is_null());
}

#[test]
fn test_funcref_to_u64() {
    let funcref = FuncRef { funcref: Some(Func { store: 1, index: NonZeroU32::new(2).unwrap() }) };
    assert_eq!(funcref, u64_to_funcref(funcref_to_u64(funcref)));
}

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 a pull request may close this issue.

1 participant