-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Implement extensible syscall interface for wasm #47102
Merged
+365
−198
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I guess this means the
std
will have to be recompiled to use this? ):To be honest I would really prefer another way of opting into this than having to recompile the whole
std
. I really don't want to have to add support forstd
recompilation tocargo-web
on top of the gazzilion of hoops I already have to go through to get something usable. ):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it will be fairly easy to use Xargo to build libstd with the additional feature, but I don't now how that will inter-operate with
cargo-web
.If you find an alternative way to dynamically configure whether or not
libstd
imports a symbol, then please suggest it, but the way this is done currently for things like the allocator/panic selection is a hack, and @alexcrichton was reluctant to build on this hack for something specific to wasm.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand the appoach used for allocators (etc.) is something like this in a nutshell: in
std
you put anextern
for a function, and then at link time you have two crates which implement that symbol and you link to one of them based on what the user wants. Correct? And this is the approach you wanted to originally take (but with only one crate instead of two, to force the generation of an import.)To me personally this doesn't really feel like a hack, and seems like the simplest way to do it. Anything I can think of is going to be more complex than that, so I guess if Alex is against it then that's it. ):
So if we're going to oficially require a recompilation of the
std
here's a request from me - @alexcrichton could we at least define the empty syscall wrapper inside ofstd
like this?Before:
After:
Basically, make it not inlineable and give it a unique name. This puts zero maintenance burden on Rust developers and will allow me to trivially support this in
cargo-web
without having to recompile thestd
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My personal feeling is that we should explicitly allow people to compile their own wasm stds easily - this PR is deliberately minimal and it's not hard to imagine people wanting to hook up additional bits and pieces (e.g. FS calls to a virtual filesystem in JS, tcp sockets to websockets). But to do this we need to make it dead easy to compile these different stds for different use-cases.
I'd personally prefer all the changes to the shim and to libstd to end up in a third party repository rather than being landed here so that it will (as I put it in my previous comment) set the scene for other people to get involved.
I am looking at Xargo at the moment and will continue tomorrow - potentially this shouldn't require much extra effort from cargo web, since the heavy lifting should be done by Xargo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate what this would do/how it would help you? If nothing else, the answer should probably be in a comment for the function :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't really mind that much if it was oficially supported by cargo/rust (e.g. you could just put the
std
in your[dependencies]
section), but unfortunately it's not yet.Sure. Currently to support the
js!
macro from stdweb I'm already doing a bunch of transformations on the WebAssembly bytecode which Rust generates. One of the things I can do with my current infrastructure is to freely remap the functions as I see fit and modify the import tables, so if I'll be able to identify which exact function in the bytecode is the syscall wrapper then I can just replace it with an import in something like ~15 lines of code. Rust itself can keep the status quo. Everyone's happy.