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.

Sign up for GitHub

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

Jump to bottom

Different output when running the same wasm file in aot mode #7558

Closed
XinyuShe opened this issue Nov 20, 2023 · 4 comments · Fixed by #7559
Closed

Different output when running the same wasm file in aot mode #7558

XinyuShe opened this issue Nov 20, 2023 · 4 comments · Fixed by #7559

Comments

@XinyuShe
Copy link

XinyuShe commented Nov 20, 2023 •
edited
Loading

filea11.zip is a simd file randomly constructed, without a source file. And I test it in AOT mode
image

Because I used an old version of wasmtime in the above picture, I downloaded the latest version of wasmtime, 14.0.4, and retested, but the results did not change. I also tested the latest versions of wasmer and wamr, both of which were different from wasmtime.

wasmtime

root@22a831657e73:# ../wasmtime --version
wasmtime-cli 14.0.4
root@22a831657e73:a# ../wasmtime --allow-precompiled --invoke main filea11.cwasm
Error: failed to run main module `filea11.cwasm`

Caused by:
    0: failed to instantiate "filea11.cwasm"
    1: wasm trap: out of bounds memory access

wasmer

>wasmer.exe run -e main filea11.wasm
24695612705472451393634029802101417489 24695612705472451393634029802101417489 24695612705472451393634029802101417489 24695612705472451393634029802101417489 24695612705472451393634029802101417489 24695612705472451393634029802101417489 24695612705472451393634029802101417489 24695612705472451393634029802101417489
>wasmer --version
wasmer 4.2.3

wamr

root@4252f5ec38df:/home/sxy/exp# iwasm --version
iwasm 1.2.3
root@4252f5ec38df:/home/sxy/exp# iwasm --heap-size=0 -f main filea11.wasm 
<0xcf1af7a4d7a82611 0x129433b64d1d090d>:v128,<0xcf1af7a4d7a82611 0x129433b64d1d090d>:v128,<0xcf1af7a4d7a82611 0x129433b64d1d090d>:v128,<0xcf1af7a4d7a82611 0x129433b64d1d090d>:v128,<0xcf1af7a4d7a82611 0x129433b64d1d090d>:v128,<0xcf1af7a4d7a82611 0x129433b64d1d090d>:v128,<0xcf1af7a4d7a82611 0x129433b64d1d090d>:v128,<0xcf1af7a4d7a82611 0x129433b64d1d090d>:v128
@tschneidereit
Copy link
Member

This isn't related to AOT compilation: the same error occurs when running the wasm file directly.

The issue seems to be that the wasm file contains a data segment with a negative offset of -79158787. By my reading of the spec, wasmtime's behavior is correct here. I'm not sure what Wasmer and WAMR are doing to get anything running at all.

alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Nov 20, 2023
@alexcrichton
Fix memory initialization when offset is negative
18679a5 
This commit fixes a bug in initializing memory segments of 32-bit
memories where if the offset was negative when viewed as a signed
integer the offset was incorrectly sign-extended to a 64-bit value
instead of zero-extended. This commit replaces an `i32`-to-`u64` cast
with an `i32`-to-`u32` cast followed by a `u32`-to-`u64` cast which
performs the zero extend.

Closes bytecodealliance#7558
@alexcrichton alexcrichton mentioned this issue Nov 20, 2023
Merged
@alexcrichton
Copy link
Member

Ah actually this is indeed a bug in Wasmtime (fixed here). The negative offset should be interpreted as a 32-bit unsigned value and because the memory here is of maximal size (4G) it should be valid. The bug was that an integer cast in Wasmtime sign-extended the value to 64-bits instead of zero-extending which produced the error seen here.

@tschneidereit
Copy link
Member

wow, that is a fascinating and surprising wrinkle of the spec. Apologies for the wrong answer @XinyuShe, and thank you for the correction and bugfix, @alexcrichton!

@XinyuShe
Copy link
Author

That's okay, I'm happy to be able to help you all. @tschneidereit

github-merge-queue bot pushed a commit that referenced this issue Nov 20, 2023
@alexcrichton
Fix memory initialization when offset is negative (#7559)
05eadca 
This commit fixes a bug in initializing memory segments of 32-bit
memories where if the offset was negative when viewed as a signed
integer the offset was incorrectly sign-extended to a 64-bit value
instead of zero-extended. This commit replaces an `i32`-to-`u64` cast
with an `i32`-to-`u32` cast followed by a `u32`-to-`u64` cast which
performs the zero extend.

Closes #7558
alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Nov 20, 2023
@alexcrichton
Selectively enable clippy::cast_sign_loss
ff4722c 
This would have fixed bytecodealliance#7558 so try to head off future issues with that
by warning against this situation in a few crates. This lint is still
quite noisy though for Cranelift for example so it's not worthwhile at
this time to enable it for the whole workspace.
alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Nov 20, 2023
@alexcrichton
Selectively enable clippy::cast_sign_loss
ac6b90f 
This would have fixed bytecodealliance#7558 so try to head off future issues with that
by warning against this situation in a few crates. This lint is still
quite noisy though for Cranelift for example so it's not worthwhile at
this time to enable it for the whole workspace.
@alexcrichton alexcrichton mentioned this issue Nov 20, 2023
Merged
github-merge-queue bot pushed a commit that referenced this issue Nov 20, 2023
@alexcrichton
Configure workspace lints, enable running some Clippy lints on CI (#7561
5856590 
)

* Configure Rust lints at the workspace level

This commit adds necessary configuration knobs to have lints configured
at the workspace level in Wasmtime rather than the crate level. This
uses a feature of Cargo first released with 1.74.0 (last week) of the
`[workspace.lints]` table. This should help create a more consistent set
of lints applied across all crates in our workspace in addition to
possibly running select clippy lints on CI as well.

* Move `unused_extern_crates` to the workspace level

This commit configures a `deny` lint level for the
`unused_extern_crates` lint to the workspace level rather than the
previous configuration at the individual crate level.

* Move `trivial_numeric_casts` to workspace level

* Change workspace lint levels to `warn`

CI will ensure that these don't get checked into the codebase and
otherwise provide fewer speed bumps for in-process development.

* Move `unstable_features` lint to workspace level

* Move `unused_import_braces` lint to workspace level

* Start running Clippy on CI

This commit configures our CI to run `cargo clippy --workspace` for all
merged PRs. Historically this hasn't been all the feasible due to the
amount of configuration required to control the number of warnings on
CI, but with Cargo's new `[lint]` table it's possible to have a
one-liner to silence all lints from Clippy by default. This commit by
default sets the `all` lint in Clippy to `allow` to by-default disable
warnings from Clippy. The goal of this PR is to enable selective access
to Clippy lints for Wasmtime on CI.

* Selectively enable `clippy::cast_sign_loss`

This would have fixed #7558 so try to head off future issues with that
by warning against this situation in a few crates. This lint is still
quite noisy though for Cranelift for example so it's not worthwhile at
this time to enable it for the whole workspace.

* Fix CI error

prtest:full
@XinyuShe XinyuShe mentioned this issue Nov 29, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix memory initialization when offset is negative alexcrichton/wasmtime
3 participants
@tschneidereit @alexcrichton @XinyuShe