Skip to content

[WasmFS] Fix standalone wasm printing, newline instead of 0 #16076

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
Merged
merged 5 commits into from
Jan 25, 2022
Merged

[WasmFS] Fix standalone wasm printing, newline instead of 0 #16076

merged 5 commits into from
Jan 25, 2022

Conversation

kripken
Copy link
Member

@kripken kripken commented Jan 21, 2022

0 works in some places but at least in wasmtime it does not.

@kripken kripken requested review from sbc100 and tlively January 21, 2022 00:25
sbc100
sbc100 reviewed Jan 21, 2022
View reviewed changes
tlively
tlively approved these changes Jan 21, 2022
View reviewed changes
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to not append any new characters and just pass the user data through untouched?

If not, LGTM 👍

@sbc100
Copy link
Collaborator

sbc100 commented Jan 21, 2022

Would it make sense to not append any new characters and just pass the user data through untouched?

If not, LGTM +1

No, I was confused at first too, but I think this function is used to implement emscripten_log and emscripten_err which always print entire lines (like puts). Thats why I suggested it be renamed.

@kripken kripken enabled auto-merge (squash) January 21, 2022 19:22
@kripken
Copy link
Member Author

kripken commented Jan 21, 2022

The old wasmtime fails here. I was testing on a newer one locally which is why I didn't notice before now.

Bump the version from 0.8.0 to 0.18.0. The newer version no longer has the bug, and the test should pass.

@kripken
Copy link
Member Author

kripken commented Jan 21, 2022 •
edited
Loading

The new wasmtime now warns when sent an invalid clock_id. The extra warning makes the test fail. Bumping to latest 0.33.0, that now aborts the entire VM on an invalid clock_id. @sbc100 you follow WASI, is that correct? I can't seem to find online docs for what the WASI clock APIs should do (I find what the functions are, but not how they should handle errors). I would expect an invalid input to lead to an error code (0.8.0) not a printed warning (0.18.0) or a VM fault (0.33.0)...

@sbc100
Copy link
Collaborator

sbc100 commented Jan 22, 2022

The new wasmtime now warns when sent an invalid clock_id. The extra warning makes the test fail. Bumping to latest 0.33.0, that now aborts the entire VM on an invalid clock_id. @sbc100 you follow WASI, is that correct? I can't seem to find online docs for what the WASI clock APIs should do (I find what the functions are, but not how they should handle errors). I would expect an invalid input to lead to an error code (0.8.0) not a printed warning (0.18.0) or a VM fault (0.33.0)...

Hmm.. I don't remember now. Do you want me to take a look at doing the wasmtime update as a separate PR? Presumably we can fix the issue by not passing an invalid value for clock_id?

@kripken
Copy link
Member Author

kripken commented Jan 22, 2022

We could work around it, but that would increase code size which sounds unfortunate if it is a wasmtime bug...

I'll file an issue there.

@kripken kripken mentioned this pull request Jan 22, 2022
Closed
@kripken
Copy link
Member Author

kripken commented Jan 22, 2022

Filed bytecodealliance/wasmtime#3714

I guess let's hold off on this PR and/or upgrading wasmtime for now.

sbc100 added a commit that referenced this pull request Jan 22, 2022
@sbc100
Update version of wasmtime used in CI
5df7a57 
Includes added couple of extra checks when calling `__wasi_clock`
functions. These only apply to standlone builds as the non-standalone
builds implement these functions directly in JS.

See #16076
@sbc100 sbc100 mentioned this pull request Jan 22, 2022
Merged
@sbc100
Copy link
Collaborator

sbc100 commented Jan 22, 2022

I created a standalone PR work around the issue: #16091

It only effects STANDALONE_WASM mode so I don't think two extra conditional should worry us.

sbc100 added a commit that referenced this pull request Jan 22, 2022
@sbc100
Update version of wasmtime used in CI
a279f87 
Includes added couple of extra checks when calling `__wasi_clock`
functions. These only apply to standlone builds as the non-standalone
builds implement these functions directly in JS.

See #16076
sbc100 added a commit that referenced this pull request Jan 22, 2022
@sbc100
Update version of wasmtime used in CI (#16091)
9a0fa6b 
Includes added couple of extra checks when calling `__wasi_clock`
functions. These only apply to standlone builds as the non-standalone
builds implement these functions directly in JS.

See #16076
@kripken kripken merged commit 4e130e4 into main Jan 25, 2022
@kripken kripken deleted the wfstnew branch January 25, 2022 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbc100 sbc100 sbc100 approved these changes

@tlively tlively tlively approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kripken @sbc100 @tlively