wasmtime / PR #10593 Some fixes for try_call · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #10593 Some fixes for try_call

Wasmtime GitHub notifications bot (Apr 16 2025 at 12:15):

bjorn3 opened PR #10593 from bjorn3:fix_try_call to bytecodealliance:main:

Together with a change to allow the system_v call conv, this is enough to pass as much of cg_clif's test suite as is expected given how much I wired up on the cg_clif side.

The second commit is not strictly necessary for correctness, but does make the optimizer work better with try_call.

Wasmtime GitHub notifications bot (Apr 16 2025 at 12:15):

bjorn3 requested fitzgen for a review on PR #10593.

Wasmtime GitHub notifications bot (Apr 16 2025 at 12:15):

bjorn3 requested wasmtime-compiler-reviewers for a review on PR #10593.

Wasmtime GitHub notifications bot (Apr 16 2025 at 13:34):

bjorn3 edited PR #10593:

~~Together with a change to allow the system_v call conv, this is enough to pass as much of cg_clif's test suite as is expected given how much I wired up on the cg_clif side.~~

Edit: Seems like rust's bootstrap executable still gets miscompiled, will fix in a later PR.

The second commit is not strictly necessary for correctness, but does make the optimizer work better with try_call.

Wasmtime GitHub notifications bot (Apr 16 2025 at 14:49):

bjorn3 edited PR #10593:

~~Together with a change to allow the system_v call conv, this is enough to pass as much of cg_clif's test suite as is expected given how much I wired up on the cg_clif side.~~

The second commit is not strictly necessary for correctness, but does make the optimizer work better with try_call.

Wasmtime GitHub notifications bot (Apr 16 2025 at 14:51):

bjorn3 commented on PR #10593:

The following gets miscompiled even with this PR (will fix in a future PR, but leaving my debugging notes here):

use std::ffi::CString;

fn main() {
    let s = CString::new(b"ar");
    panic!("{s:?}");
}

Reduced so far to:

pub struct MyCString {
    inner: Box<[u8]>,
}

impl MyCString {
    #[inline(never)]
    pub fn new(bytes: &[u8]) -> MyCString {
        let buffer = bytes.to_vec();

        let mut buffer = buffer;
        buffer.push(0);
        Self { inner: buffer.into_boxed_slice() }
    }
}

fn main() {
    let s = MyCString::new(b"ar");
    panic!("{:?}", s.inner); // nul-terminator gets lost
}

Wasmtime GitHub notifications bot (Apr 16 2025 at 14:52):

bjorn3 edited a comment on PR #10593:

The following gets miscompiled even with this PR (will fix in a future PR, but leaving my debugging notes here):

use std::ffi::CString;

fn main() {
    let s = CString::new(b"ar");
    panic!("{s:?}");
}

Reduced so far to:

pub struct MyCString {
    inner: Vec<u8>,
}

impl MyCString {
    #[inline(never)]
    pub fn new(bytes: &[u8]) -> MyCString {
        let buffer = bytes.to_vec();

        let mut buffer = buffer;
        buffer.push(0);
        Self { inner: buffer }
    }
}

fn main() {
    let s = MyCString::new(b"ar");
    panic!("{:?}", s.inner); // nul-terminator gets lost
}

Wasmtime GitHub notifications bot (Apr 16 2025 at 14:52):

bjorn3 edited a comment on PR #10593:

The following gets miscompiled even with this PR in release mode (will fix in a future PR, but leaving my debugging notes here):

use std::ffi::CString;

fn main() {
    let s = CString::new(b"ar");
    panic!("{s:?}");
}

Reduced so far to:

pub struct MyCString {
    inner: Vec<u8>,
}

impl MyCString {
    #[inline(never)]
    pub fn new(bytes: &[u8]) -> MyCString {
        let buffer = bytes.to_vec();

        let mut buffer = buffer;
        buffer.push(0);
        Self { inner: buffer }
    }
}

fn main() {
    let s = MyCString::new(b"ar");
    panic!("{:?}", s.inner); // nul-terminator gets lost
}

Wasmtime GitHub notifications bot (Apr 16 2025 at 15:27):

fitzgen submitted PR review:

LGTM, but we should have a test for the first commit before this merges. Thanks!

Wasmtime GitHub notifications bot (Apr 16 2025 at 15:27):

fitzgen created PR review comment:

Mind adding a simple test for a try_call where the same successor appears multiple times?

Wasmtime GitHub notifications bot (Apr 16 2025 at 15:27):

fitzgen created PR review comment:

This pass is used by cg_clif, I assume? We should probably have filetests for these changes, but I see that we don't have the infrastructure to run this pass on filetests at the moment, and I don't want to necessarily block on that. But longer term, we should either gain that functionality, so that we can hold it to our usual code standards, or else this pass should move into cg_clif (since I don't believe it relies on anything that isn't publicly exported).

Wasmtime GitHub notifications bot (Apr 16 2025 at 15:30):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Apr 16 2025 at 15:30):

bjorn3 created PR review comment:

This pass is part of the normal optimization pipeline of Cranelift and runs unconditionally.

Wasmtime GitHub notifications bot (Apr 16 2025 at 16:15):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 16 2025 at 16:15):

fitzgen created PR review comment:

Huh. rust-analyzer wasn't finding uses of it for me.

Do we exercise it under the test optimize filetest mode? If so, can you add a filetest for it?

Wasmtime GitHub notifications bot (Apr 17 2025 at 11:27):

bjorn3 edited a comment on PR #10593:

The following gets miscompiled even with this PR in release mode (will fix in a future PR, but leaving my debugging notes here):

use std::ffi::CString;

fn main() {
    let s = CString::new(b"ar");
    panic!("{s:?}");
}

Reduced so far to:

pub struct MyCString {
    inner: Vec<u8>,
}

#[inline(never)]
fn push(v: &mut Vec<u8>) {
    v.push(0);
}

impl MyCString {
    #[inline(never)]
    pub fn new() -> MyCString {
        let buffer = Vec::new();

        let mut buffer = buffer;
        push(&mut buffer);
        Self { inner: buffer }
    }
}

fn main() {
    let s = MyCString::new();
    panic!("{:?}", s.inner); // nul-terminator gets lost
}

Wasmtime GitHub notifications bot (Apr 17 2025 at 11:30):

bjorn3 edited a comment on PR #10593:

The following gets miscompiled even with this PR in release mode (will fix in a future PR, but leaving my debugging notes here):

use std::ffi::CString;

fn main() {
    let s = CString::new(b"ar");
    panic!("{s:?}");
}

Reduced so far to:

pub struct MyCString {
    inner: Vec<u8>,
}

#[inline(never)]
fn push(v: &mut Vec<u8>) {
    v.push(0);
}

impl MyCString {
    #[inline(never)]
    pub fn new() -> MyCString {
        let mut buffer = const { Vec::new() };
        push(&mut buffer);
        Self { inner: buffer }
    }
}

fn main() {
    let s = MyCString::new();
    panic!("{:?}", s.inner); // nul-terminator gets lost
}

Wasmtime GitHub notifications bot (Apr 17 2025 at 11:31):

bjorn3 edited a comment on PR #10593:

The following gets miscompiled even with this PR in release mode (will fix in a future PR, but leaving my debugging notes here):

use std::ffi::CString;

fn main() {
    let s = CString::new(b"ar");
    panic!("{s:?}");
}

Reduced so far to:

pub struct MyCString {
    inner: Vec<u8>,
}

#[inline(never)]
fn push(v: &mut Vec<u8>) {
    v.push(0);
}

impl MyCString {
    #[inline(never)]
    pub fn new() -> MyCString {
        let mut buffer = const { MyCString { inner: Vec::new() } };
        push(&mut buffer.inner);
        buffer
    }
}

fn main() {
    let s = MyCString::new();
    panic!("{:?}", s.inner); // nul-terminator gets lost
}

Wasmtime GitHub notifications bot (Apr 17 2025 at 13:32):

bjorn3 updated PR #10593.

Wasmtime GitHub notifications bot (Apr 17 2025 at 13:34):

bjorn3 edited a comment on PR #10593:

<details><summary>Fixed miscompilation</summary>

The following gets miscompiled even with this PR in release mode (will fix in a future PR, but leaving my debugging notes here):

use std::ffi::CString;

fn main() {
    let s = CString::new(b"ar");
    panic!("{s:?}");
}

Reduced so far to:

pub struct MyCString {
    inner: Vec<u8>,
}

#[inline(never)]
fn push(v: &mut Vec<u8>) {
    v.push(0);
}

impl MyCString {
    #[inline(never)]
    pub fn new() -> MyCString {
        let mut buffer = const { MyCString { inner: Vec::new() } };
        push(&mut buffer.inner);
        buffer
    }
}

fn main() {
    let s = MyCString::new();
    panic!("{:?}", s.inner); // nul-terminator gets lost
}

</details>

Edit: Pushed a commit to fix this miscompilation.

Wasmtime GitHub notifications bot (Apr 17 2025 at 13:35):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Apr 17 2025 at 13:35):

bjorn3 created PR review comment:

Context::optimize contains an unconditional call to self.eliminate_unreachable_code(isa)?; which in turn calls this function.

Wasmtime GitHub notifications bot (Apr 17 2025 at 13:58):

bjorn3 updated PR #10593.

Wasmtime GitHub notifications bot (Apr 17 2025 at 14:02):

bjorn3 edited PR #10593:

Together with a change to allow the system_v call conv, this is enough to pass most of cg_clif's test suite (including panicking tests). In addition rustc's compiletest now works just fine in panic=unwind mode. Not all rustc tests pass yet, but the test failures seem to be caused by either cg_clif or bugs in the tests themself.

The second commit is not strictly necessary for correctness, but does make the optimizer work better with try_call.

Wasmtime GitHub notifications bot (Apr 17 2025 at 14:09):

bjorn3 updated PR #10593.

Wasmtime GitHub notifications bot (Apr 17 2025 at 14:30):

bjorn3 updated PR #10593.

Wasmtime GitHub notifications bot (Apr 17 2025 at 15:00):

bjorn3 updated PR #10593.

Wasmtime GitHub notifications bot (Apr 17 2025 at 15:01):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Apr 17 2025 at 15:01):

bjorn3 created PR review comment:

Added a test. Also saw that there was a debug assertion that would have caused it. I've upgraded it to a regular assertion as it should be cheap.

Wasmtime GitHub notifications bot (Apr 17 2025 at 15:01):

bjorn3 edited PR review comment.

Wasmtime GitHub notifications bot (Apr 17 2025 at 15:11):

bjorn3 updated PR #10593.

Wasmtime GitHub notifications bot (Apr 17 2025 at 15:13):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Apr 17 2025 at 15:13):

bjorn3 created PR review comment:

As for testing, I'm not sure how to do that given that the clif ir printing doesn't show a list of all exception tables, but rather only shows them as part of a try_call instruction.

Wasmtime GitHub notifications bot (Apr 17 2025 at 15:20):

bjorn3 updated PR #10593.

Wasmtime GitHub notifications bot (Apr 17 2025 at 15:46):

cfallin submitted PR review:

LGTM on the additional commits -- just one thought on the verifier change below. I'm excited that you found the memory-fence issue and have things working with cg_clif now!

Wasmtime GitHub notifications bot (Apr 17 2025 at 15:46):

cfallin created PR review comment:

Perhaps we could continue here so we still check the remaining args?

Wasmtime GitHub notifications bot (Apr 17 2025 at 15:50):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Apr 17 2025 at 15:50):

bjorn3 created PR review comment:

Right, will fix.

Wasmtime GitHub notifications bot (Apr 17 2025 at 15:52):

bjorn3 updated PR #10593.

Wasmtime GitHub notifications bot (Apr 17 2025 at 15:52):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Apr 17 2025 at 15:52):

bjorn3 created PR review comment:

Done

Wasmtime GitHub notifications bot (Apr 17 2025 at 16:15):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Apr 17 2025 at 16:15):

cfallin created PR review comment:

I think that's correct -- I can't see a way to test for dangling exception tables either unless we change the print format. I think this is probably fine to merge as-is -- the other direction (avoiding false-removals of actually used exception tables) is the more important one and is implicitly tested.

Wasmtime GitHub notifications bot (Apr 17 2025 at 16:16):

cfallin commented on PR #10593:

Looked over all commits and this looks good -- I'm going to do a short followup based on @bjorn3's cg_clif unwinding enabling work, so I'll go ahead and merge this now.

Wasmtime GitHub notifications bot (Apr 17 2025 at 16:37):

cfallin merged PR #10593.

Last updated: Dec 06 2025 at 07:03 UTC