wasm-interp: Add multi-module support #2009

tDwtp · 2022-09-28T21:41:30Z

#2005 feature is finally implemented.
Modules can be loaded via -m --module=NAME:PATH
Maybe I will expand it in the future to accept dll/so/dylib/bundle as well. I am open for suggestions.
-N --native=mod:lib.so would be nice, or make --module more dynamic? *
'@' will override the module name
'$' is reserved to prefer prefer the debug name if provided in the name section.
providing no NAME use the file-name. again reserved for the future is to use the debug name given in the name section

* note that native libraries either need some witx glue, or ffi or an API to bind against.

module-loading works, but it will give an error! (and crash)

unnecessary lines were removed, mostly commented out stuff for testing. guarded the exports, so it wont throw an error with `--dummy-import-func`

clear maps again. fixes error on exit (and removved excess whitespace)

removed the hint, for `BindImports`to do the same as `PopulateImports``

Clear the maps in the proper place. Also removed more notes, and expanded the documentation

linting and improve documentation. test now include new module directive, too.

tDwtp · 2022-09-29T14:25:45Z

How can I write a test, to check for it to work porperly.
I found how to run app.wasm, but how can I provide a second file?

sbc100 · 2022-09-30T05:45:02Z

src/tools/wasm-interp.cc

@@ -110,6 +117,13 @@ static void ParseOptions(int argc, char** argv) {
 parser.AddOption(
 'd', "dir", "DIR", "Pass the given directory the the WASI runtime",
 [](const std::string& argument) { s_wasi_dirs.push_back(argument); });
+ parser.AddOption(
+ 'm', "module", "[(@|$)NAME:]FILE",


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?

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

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.

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.

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.

src/tools/wasm-interp.cc

sbc100 · 2022-09-30T05:50:47Z

src/tools/wasm-interp.cc

@@ -239,6 +346,37 @@ static Result ReadAndRunModule(const char* module_filename) {

 RefVec imports;

+ // if we only have dummy imports, we wont need to reghister anything
+ // but we still want to load them.
+ std::vector<Module::Ptr> modules_loaded(s_modules.size());


Can we unify the loading of the main module_filename above with the loading of s_modules here?

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.

Should I add a Divider to Option Parser?

parser.AddArgument("filename", OptionParser::ArgumentCount::OneOrMore, [...]); // parser.AddDivider("--", OptionParser::Divider::Optional); parser.AddArgument("arg", OptionParser::ArgumentCount::ZeroOrMore, [...]);

This way I could distinguish between modules and wasi arguments.

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.

Well: Off I go to find out how to implement that... Possibly wihtout implementing a whole Divider thing.

sbc100 · 2022-09-30T05:58:47Z

src/tools/wasm-interp.cc

+
+ // return the override_name (remove leading '$' if present)
+ bool leading_dollar = override_name[0] == '$';
+ return override_name.substr(leading_dollar ? 1 : 0);


Do we need this dollar sign syntax? What about this simpler scheme:

Override name, if present

Debug name, if present

Fall back to file name

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 present

if override_name == "$" debug_name if present

Fall back to file_name

if (!override_name.empty()) { // prefer override_name unless its just "$" if (override_name != "$") { return override_name; } // prefer debug_name if override_name == "$" if (!debug_name.empty()) { return debug_name; } } // fall back to file-name return FileNameOfPath(path_name);

or we go for this:

if override_name == "@" use file_name

overriide_name if present

debug_name if present

Fall back to file_name

Binary compatibility would then be prefered

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.

was just for conveniance... But tbh I don't care to much.
Its done already. I removed the magic thingy.

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.

@sbc100 minor question, wouldn't it make sense to just ... use all three? Like: Always register by file_name, if exists register by debug_name, if exists register by override_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)

src/tools/wasm-interp.cc

sbc100

I think the main thing we need to do before landing this is figure out a way to start testing this.

We can bikeshed the name of the argument as a followup. I would like to remove the magic @ syntax though, since that seems unnecessary for v1.

oops

made wasi work again. older version would only allow wasi in last module.

help is now updated to reflect the changes

tDwtp · 2022-10-01T17:24:07Z

wasi is working now. and i change unordered_map and the file fallback.

I really tried to make the arguments work, but I would need another argument to AddArgument.

parser.AddArgument("arg-name", OptionParser, [callback], std::string stopper);

I could also just add it to OneOrMore and ZeroOrMore and generally assume its . as i assumed for my case.
It cannot be -- as this is handled by the parser itself! It does not seem worth the hassle.
I am not fluent enough in all shells to attempt it. I'd pick between +, . or , but I really dont know.
I picked . in my hack because it seemed most sensible.

sbc100 · 2022-10-01T17:31:36Z

What do you think about renaming this PR to "wasm-interp: Add multi-module support"?

sbc100 · 2022-10-01T17:41:28Z

wasi is working now. and i change unordered_map and the file fallback.

I really tried to make the arguments work, but I would need another argument to AddArgument.
parser.AddArgument("arg-name", OptionParser, [callback], std::string stopper);
I could also just add it to OneOrMore and ZeroOrMore and generally assume its . as i assumed for my case. It cannot be -- as this is handled by the parser itself! It does not seem worth the hassle. I am not fluent enough in all shells to attempt it. I'd pick between +, . or , but I really dont know. I picked . in my hack because it seemed most sensible.

Awesome, I don't think we need to worry about getting the option separator thing in as part of this change.

sbc100 · 2022-10-01T17:44:01Z

src/tools/wasm-interp.cc

- s_trace_stream->Writef("wasi: arg: \"%s\"\n", s.c_str());
- }
- argv.push_back(s.c_str());
+ result = InitWasi(&uvwasi);


What do you think aInitWasibout making a separate NFC (non-functional change) which would just refactor all this wasi stuff. i.e. InitWasi, RegisterWasiInstance, etc?

That would make this change much simpler to understand I think.

sbc100 · 2022-10-01T17:45:11Z

src/interp/interp-wasi.h

+ Stream* stream,
+ Stream* trace_stream);
+
+void UnregisterWasiInstance(const Instance::Ptr& instance);


Can we make all these function symbols start with Wasi?

tDwtp · 2022-10-01T17:48:44Z

Don't have much time for today,

We can bikeshed the name of the argument as a followup. I would like to remove the magic @ syntax though, since that seems unnecessary for v1.

Will revert the hack later.

I think the main thing we need to do before landing this is figure out a way to start testing this.

I have a simple test ready. will add 4 more simple ones at least. Already have them in mind.
I convert a wat file to wasm and use it for module imports. I just don't know where to put them and how the testing works exactly.

CU tomorrow.

rsolving conflicts

tDwtp · 2022-10-14T16:52:16Z

I... do not remember how I had planned the tests... I need help again.

sbc100 · 2022-10-14T17:03:18Z

src/interp/interp-wasi.cc

@@ -35,6 +35,7 @@

 #include "uvwasi.h"

+#include <cstdio>


Is this needed?

was for testing and finding a memory-leak culprit. is removed. Thanks for the notice ._.

sbc100 · 2022-10-14T17:04:17Z

src/tools/wasm-interp.cc

 }
 #endif
+ // unregister all;
+ for (auto&& instance : instance_loaded) {


Is the double & needed here?

sbc100 · 2022-10-14T17:08:11Z

src/tools/wasm-interp.cc

+ return module_arg.substr(path_at);
+}
+
+static std::string FileNameOfPath(std::string path_name) {


I think you can use GetBasename + StripExtension from wabt/filenames.h?

Thanks for the notice. I did not even know that exists already.

sbc100 · 2022-10-14T17:10:58Z

src/interp/interp-wasi.cc

+ }
+
+ WasiInstance* wasi = new WasiInstance(instance, uvwasi, std::move(memory).get(), trace_stream);
+ wasiInstances[instance.get()] = std::move(wasi);


I don't think std::move is meaningful on raw pointer.

Maybe just avoid the local and do wasiInstances[instance.get()] = new WasiInstance...

sbc100 · 2022-10-14T17:11:47Z

src/interp/interp-wasi.cc

- goto found; \
+void WasiUnregisterInstance(const Instance::Ptr& instance) {
+ WasiInstance* wasi = wasiInstances[instance.get()];
+ if (wasi) {


How about assert(wasi); and skip the if check?

It might already be deleted. The main file gets a delete before this happens, which might not be cleared, if _start was not found or used. See WasiRunStart. Its just for later, when I might load and clear without any order.

sbc100 · 2022-10-14T17:13:48Z

src/interp/interp-wasi.cc

@@ -753,7 +756,7 @@ Result WasiRunStart(const Instance::Ptr& instance,
 }

 // Unregister memory
- wasiInstances.erase(instance.get());
+ UnregisterWasiInstance(instance);


This needs to be renamed I think.

Should this line be removed now that the registration and unregistration now all seems to be taken care in wasm-interp.cc?

Nope, You might use it elsewhere (in spec-test for example). I did not want to monolith wasm-interp.cc.

EDIT: also see: WasiRunStart

adjust whitespace and indentation according to linter

…odule-loading

resorted includes according to linter

corrected and expanded based on the comments of WebAssembly#2009 removed redundant function and adjusted imports.

forgot to check builds without wasi support.

I added back in `called host` which I removed to better find debugging prompts.

removed the dot which was to distinguish between arguments and modules

tDwtp · 2022-10-14T23:45:56Z

sorry for the many mails... I am still setting stuff up, I moved and did not have my pc set up untill now.
I still have not set up my dev enironment properly...

tDwtp added 5 commits September 28, 2022 22:14

module-loading works, but errors!

d53a1e9

module-loading works, but it will give an error! (and crash)

removed unnecessary lines and guarded exports

b9da00b

unnecessary lines were removed, mostly commented out stuff for testing. guarded the exports, so it wont throw an error with `--dummy-import-func`

forgot to clear the maps

ff68a0b

clear maps again. fixes error on exit (and removved excess whitespace)

no hints needed anymore

f1d7371

removed the hint, for `BindImports`to do the same as `PopulateImports``

removed more notes expanded documentation properly clear the maps

86eb626

Clear the maps in the proper place. Also removed more notes, and expanded the documentation

tDwtp mentioned this pull request Sep 28, 2022

Load other wasm (and possibly native) modules for exposing exports/imports #2005

Closed

linting and better doc, test expanded

daab8ed

linting and improve documentation. test now include new module directive, too.

sbc100 changed the title ~~Module loading is now implemented~~ wasm-interp: Implement module loading Sep 30, 2022

sbc100 reviewed Sep 30, 2022

View reviewed changes

src/tools/wasm-interp.cc Show resolved Hide resolved

sbc100 previously approved these changes Sep 30, 2022

View reviewed changes

tDwtp added 2 commits October 1, 2022 19:01

wasi working again

d77a735

made wasi work again. older version would only allow wasi in last module.

updated help and test file

b2a63ba

help is now updated to reflect the changes

sbc100 reviewed Oct 1, 2022

View reviewed changes

tDwtp changed the title ~~wasm-interp: Implement module loading~~ wasm-interp: Add multi-module support Oct 14, 2022

tDwtp added 2 commits October 14, 2022 18:50

resolve conflicts

307e91c

rsolving conflicts

Merge branch 'main' into module-loading

9be9d33

sbc100 reviewed Oct 14, 2022

View reviewed changes

tDwtp added 4 commits October 14, 2022 21:47

satisfy linter

d4e4833

adjust whitespace and indentation according to linter

Merge branch 'module-loading' of https://github.com/tDwtp/wabt into m…

fbe602b

…odule-loading

satisfy linter

baa0e5c

resorted includes according to linter

correction due to comments on WebAssembly#2009 and the linter

523aaa4

corrected and expanded based on the comments of WebAssembly#2009 removed redundant function and adjusted imports.

tDwtp added 7 commits October 14, 2022 22:13

type and spelling correction

18b09f1

removed the flag from argumentl ist handling

ba49f14

place InitWasi correctly inside macro-block

50db7e0

forgot to check builds without wasi support.

again fixed non-wasi build

ca6aa99

reverted something i removed for testing

7f0489a

I added back in `called host` which I removed to better find debugging prompts.

dot removed from old parsing checks

1ed7b45

removed the dot which was to distinguish between arguments and modules

linter update

f7160d2

sbc100 mentioned this pull request Nov 7, 2022

wasm2c: add hook to allow a custom trap handler #2042

Merged

wasm-interp: Add multi-module support #2009

Are you sure you want to change the base?

wasm-interp: Add multi-module support #2009

Conversation

tDwtp commented Sep 28, 2022

tDwtp commented Sep 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tDwtp Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 left a comment

Choose a reason for hiding this comment

tDwtp commented Oct 1, 2022

sbc100 commented Oct 1, 2022

sbc100 commented Oct 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tDwtp commented Oct 1, 2022

tDwtp commented Oct 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tDwtp Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tDwtp Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

tDwtp commented Oct 14, 2022

tDwtp Sep 30, 2022 •

edited

Loading

tDwtp Oct 14, 2022 •

edited

Loading

tDwtp Oct 14, 2022 •

edited

Loading