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

minimodem binary works in wasmtime, doesn't work in wasmer #2904

Closed
jcaesar opened this issue May 29, 2022 · 8 comments
Closed

minimodem binary works in wasmtime, doesn't work in wasmer #2904

jcaesar opened this issue May 29, 2022 · 8 comments
Assignees
Labels
bug Something isn't working 🕵️ needs investigation The issue/PR needs further investigation priority-low Low priority issue
Milestone

Comments

@jcaesar
Copy link
Contributor

jcaesar commented May 29, 2022

Describe the bug

This wasm-wasi binary (compiled from here) works fine on wasmtime:

echo This message will get through | wasmtime run -- minimodem.wasm --tx --tx-carrier --stdio --float-samples same | wasmtime run -- minimodem.wasm --rx --tx-carrier --stdio --float-samples same
### CARRIER 521 @ 2083.3 Hz ###
This message will get through

### NOCARRIER ndata=46 confidence=141751.797 ampl=45.977 bps=521.03 (0.0% fast) ###

But bugs on wasmer

echo This message wont get through | wasmer run -- minimodem.wasm --tx --tx-carrier --stdio --float-samples same | wasmer run -- minimodem.wasm --rx --tx-carrier --stdio --float-samples same
### CARRIER 521 @ 2083.3 Hz ###
R�iln w�4�)
### NOCARRIER ndata=36 confidence=90307.961 ampl=41.893 bps=515.86 (1.0% slow) ###

Steps to reproduce

  1. Download binary linked above
  2. Run commands shown above

Expected behavior

Shouldn't fail to decode audio

Actual behavior

Fails to decode audio

Additional context

@jcaesar jcaesar added the bug Something isn't working label May 29, 2022
@epilys epilys added 🕵️ needs investigation The issue/PR needs further investigation priority-low Low priority issue labels Jun 21, 2022
@epilys epilys added this to the v3.0 milestone Jun 21, 2022
@ptitSeb
Copy link
Contributor

ptitSeb commented Jul 26, 2022

I reproduced the issue, even with current wasmer (almost 3.0) code.

I built a native version of minimodem also, and found that only the "rx" part is broken. The "tx" part seems to be working as intended.
Also, I tried with all the compiler and the issue is still there, so probably something on the WASI side.

I'll continue my investigations.

@ptitSeb
Copy link
Contributor

ptitSeb commented Jul 27, 2022

Ok, I found the issue. It was the fd_read WASI syscall. When reading on multiple iovs, if a read is partial (so not all the wanted bytes are read), the function would still continue to try reading on subsequent iov (eventualy reading more bytes but not in the right place).

@jcaesar
Copy link
Contributor Author

jcaesar commented Jul 27, 2022

6d08c88 does indeed fix it. Ty.

bors bot added a commit that referenced this issue Jul 27, 2022
3045: Fixed WASI fd_read syscall when reading multiple iovs and read is partial (for #2904) r=ptitSeb a=ptitSeb

# Description
Fixed an issue with WASI fd_read: when reading on multiple iovs, if a read is partial (so not all the wanted bytes are read), the function would still continue to try reading on subsequent iov (eventualy reading more bytes but not in the right place).

Co-authored-by: ptitSeb <[email protected]>
@jcaesar jcaesar closed this as completed Aug 1, 2022
@jcaesar
Copy link
Contributor Author

jcaesar commented Apr 2, 2023

This exact same issue reappeared (though the cause is probably different, last time the --rx side was broken, this time it's the --tx side [Edit:] On master, it's rx that's broken again. Ookay?). Please do tell if you'd prefer I open a new issue.

I'll try to bisect it if I find the time. Should I also add a minimodem test case? It seems to be good at finding bugs.

@jcaesar jcaesar reopened this Apr 2, 2023
@ptitSeb
Copy link
Contributor

ptitSeb commented Apr 3, 2023

Ok, I tested with current master, and indeed it's borken.
I also tested "tx" and "rx" part separatly, and they seems both broken. If I redirect the "tx" part to a file, that file is different from a reference file generated wth wasmer 2.3 for example.

I'll try to digg in and see what is happening.

ptitSeb added a commit that referenced this issue Apr 3, 2023
Fixed Vectored IO when a partial operation occurs (help rx part of #2904)
@ptitSeb
Copy link
Contributor

ptitSeb commented Apr 5, 2023

The second part of the fix has been merged with #3741

@jcaesar
Copy link
Contributor Author

jcaesar commented Apr 5, 2023

Wow, fast.

@jcaesar jcaesar closed this as completed Apr 5, 2023
@ptitSeb
Copy link
Contributor

ptitSeb commented Apr 6, 2023

And it will be tested now, it's been added to snapshot tests. No more regressions :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🕵️ needs investigation The issue/PR needs further investigation priority-low Low priority issue
Projects
None yet
Development

No branches or pull requests

3 participants