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.
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
wasm-interp: Add multi-module support #2009
base: main
Are you sure you want to change the base?
wasm-interp: Add multi-module support #2009
Changes from 6 commits
d53a1e9
b9da00b
ff68a0b
f1d7371
86eb626
daab8ed
d77a735
b2a63ba
307e91c
9be9d33
d4e4833
fbe602b
baa0e5c
523aaa4
18b09f1
ba49f14
50db7e0
ca6aa99
7f0489a
1ed7b45
f7160d2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 wonder if we can come up with better name for this parameter? How about something like
--preload
?Also perhaps we could just get away without adding a new option here at all and just treat these module like main binary we are running. i.e could we just allow wasm-interp to take any number of binaries as arguments rather than the current limit of just one?
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 wanted module to be used for the import/exprot because thats what its called in the docs of WebAssembly.
--preload
would not be to far fetched, but I still think--module
(or just--mod
) would still be best. (You can choose, just wanted to give my reasoning).Sadly if we want it to interoperate with
--wasi
(and as long as we don't have a glue for native libraries) a new option is a must.I was thinking about multiple different ways, but the OptionParser does not really like them. I cannot specify multiple modules, then a main one, and then optional parameters without some new option to mark the main module. I guessed
-m
for conveniance á la '-L"path" -llibname' would be best and in the end just stuck to the-m
/--module
to be more like the other options available.I was thinking about something to glue native libraries and just have
--wasi
be an indicator which native library to give the optional arguments to.references:
https://webassembly.github.io/spec/core/valid/modules.html#imports
https://webassembly.github.io/spec/core/text/modules.html#imports
https://webassembly.github.io/spec/core/syntax/modules.html#imports
https://webassembly.github.io/spec/core/binary/modules.html#import-section
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.
Still a new option, but if I waht you want, this would be better suited:
we could have an
-a, --arg
parameter right before the arguments. I just don't know how to make it optional and still have a list of arguments following it.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 know now why I did this, only the main module has access to
--wasi
the other ones don't.I first need to figure out how to provide wasi support to all of them, then how to do the
--arg
thingy and then I don't mind.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.
Speaking of
-L
.. I wonder if we could have wasi-interp automatically looks for wasm files in certain locations and load them on demand to satisfy imports?We could even do something fancy like JS import maps: https://github.com/WICG/import-maps
But all that is farther down the line after we land this initial support.
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 think you can use
GetBasename
+StripExtension
fromwabt/filenames.h
?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.
Thanks for the notice. I did not even know that exists already.
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.
Do we need this dollar sign syntax? What about this simpler scheme:
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.
We need something to explicitly go for the debug name. I dont want something unexpected. Like downloading a module from the web, and not have it use the filename, because you might not check for the debug name. (just for convenience)
I came up with
override_name
if presentoverride_name == "$"
debug_name if presentThere 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.
or we go for this:
override_name == "@"
usefile_name
overriide_name
if presentdebug_name
if presentfile_name
Binary compatibility would then be prefered
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 just don't think we need a way to force the file name to be used over the debug name. If you really want to first the filename why not just use the override mechism. e.g -m libfoo:libfoo.wasm.
In other words, always prefer the debug name, but give users an opt out to override if they want to. That would avoid the magic
@
thing.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.
was just for conveniance... But tbh I don't care to much.
Its done already. I removed the magic thingy.
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.
Oh I also can't find a way to get the DebugName, my best guess was it's hidden somewhere in
Instance
, but I cant find it.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.
@sbc100 minor question, wouldn't it make sense to just ... use all three? Like: Always register by
file_name
, if exists register bydebug_name
, if exists register byoverride_name
.This could be an idea for a revision. And as soon as I findout how to get
debug_name
. (It seems like I need to query it manually from the file? The other approaches used in other files are ... complicated)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 we unify the loading of the main
module_filename
above with the loading ofs_modules
here?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 was thinking about it early, but dropped it, because I did not understand how it would make it work.
Probably why I have that vector in the first place.
Now on my TODO. After I figure out how to do the module option thingy.
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.
Should I add a Divider to Option Parser?
This way I could distinguish between modules and wasi arguments.
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.
Yes, I think using
--
to distinguish between arguments passed to wasi and argument to wasm-interp itself is good idea.In general I'm a fan of the direction of this PR, and I think we could probably even land with without too much more iteration, but I would also like to be able to iterate on the ergonomics of it. But perhaps we can do a lot of that iteration after first landing it.
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.
Well: Off I go to find out how to implement that... Possibly wihtout implementing a whole Divider thing.