Stream: git-wasmtime

Topic: wasmtime / Issue #2349 Fix off-by-one error looking up fr...

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2020 at 15:35):

alexcrichton commented on Issue #2349:

Oh dear, good catch! Instead of tweaking the lookup parameters it seems like we should tweak the keys in the map though? I think it might be clearer if the keys only spanned the member bytes of each function.

Also, can you add a comment for the off-by-one arithmetic in the source too?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2020 at 15:47):

uweigand commented on Issue #2349:

Oh dear, good catch! Instead of tweaking the lookup parameters it seems like we should tweak the keys in the map though? I think it might be clearer if the keys only spanned the member bytes of each function.

Also, can you add a comment for the off-by-one arithmetic in the source too?

It seems like this the place where the ranges are inserted:

    for (i, allocated, traps, address_map) in module.trap_information() {
        let (start, end) = unsafe {
            let ptr = (*allocated).as_ptr();
            let len = (*allocated).len();
            (ptr as usize, ptr as usize + len)
        };

I guess we can set end to "ptr + len - 1" instead. Do you know if we need to care about the len == 0 case here?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2020 at 15:50):

uweigand edited a comment on Issue #2349:

Oh dear, good catch! Instead of tweaking the lookup parameters it seems like we should tweak the keys in the map though? I think it might be clearer if the keys only spanned the member bytes of each function.

Also, can you add a comment for the off-by-one arithmetic in the source too?

It seems like this is the place where the ranges are inserted:

    for (i, allocated, traps, address_map) in module.trap_information() {
        let (start, end) = unsafe {
            let ptr = (*allocated).as_ptr();
            let len = (*allocated).len();
            (ptr as usize, ptr as usize + len)
        };

I guess we can set end to "ptr + len - 1" instead. Do you know if we need to care about the len == 0 case here?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2020 at 15:55):

alexcrichton commented on Issue #2349:

I think if the length is zero we can either debug assert that doesn't happen (because I don't think it should) or we could skip it since we can't execute a zero-length function anyway.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2020 at 16:08):

uweigand commented on Issue #2349:

OK, updated the patch accordingly. This also fixes the problems I was seeing.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2020 at 16:16):

alexcrichton commented on Issue #2349:

Thanks! Would it be possible to add a test for this as well? (Maybe as an inline #[test]?)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2020 at 16:31):

uweigand commented on Issue #2349:

Hmm, not sure what the best way would be here. Calling "register" even from a unit test requires having a full CompiledModule, and I'm not sure how to best create one of those with the property that two functions have immediately adjacent ranges. That would seem to depend on the specific behavior of the back-end, and be a bit fragile w.r.t. changes in the compiler in general ... Any suggestions?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2020 at 16:38):

github-actions[bot] commented on Issue #2349:

Subscribe to Label Action

cc @peterhuene

<details>
This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2020 at 23:21):

alexcrichton commented on Issue #2349:

It sounds like you were seeing this locally, so could that be checked in as a test? Otherwise we could add a small test that compiles a Module from a wasm file and then reaches into the internals and iterates over every jit byte and asserts that we get the right function out?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2020 at 09:53):

uweigand commented on Issue #2349:

It sounds like you were seeing this locally, so could that be checked in as a test?

I was seeing this with my out-of-tree IBM Z backend under development, so this wouldn't work in tree.

Otherwise we could add a small test that compiles a Module from a wasm file and then reaches into the internals and iterates over every jit byte and asserts that we get the right function out?

OK, added a test along those lines. Looks a bit tautological to me, but let me know what you think ...

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2020 at 19:54):

alexcrichton commented on Issue #2349:

Looks good to me, thanks! It at least fails before the patch and succeeds after, and looks like it won't be the hardest thing in the world to maintain.

Last updated: Dec 23 2024 at 12:05 UTC