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

wasm-interp: Add multi-module support #2009

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

tDwtp
Copy link
Contributor

@tDwtp tDwtp commented Sep 28, 2022

#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
Copy link
Contributor Author

tDwtp commented Sep 29, 2022

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 sbc100 changed the title Module loading is now implemented wasm-interp: Implement module loading Sep 30, 2022
@@ -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",
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

src/tools/wasm-interp.cc Outdated Show resolved Hide resolved
@@ -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());
Copy link
Member

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 of s_modules here?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

  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.

Copy link
Member

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.

Copy link
Contributor Author

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.


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

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:

  1. Override name, if present
  2. Debug name, if present
  3. Fall back to file name

Copy link
Contributor Author

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

  1. override_name if present
  2. if override_name == "$" debug_name if present
  3. 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);

Copy link
Contributor Author

@tDwtp tDwtp Sep 30, 2022

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:

  1. if override_name == "@" use file_name
  2. overriide_name if present
  3. debug_name if present
  4. Fall back to file_name

Binary compatibility would then be prefered

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 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)

sbc100
sbc100 previously approved these changes Sep 30, 2022
Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

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.

made wasi work again. older version would only allow wasi in last module.
help is now updated to reflect the changes
@tDwtp
Copy link
Contributor Author

tDwtp commented Oct 1, 2022

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
Copy link
Member

sbc100 commented Oct 1, 2022

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

@sbc100
Copy link
Member

sbc100 commented Oct 1, 2022

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.

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

Choose a reason for hiding this comment

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

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.

Stream* stream,
Stream* trace_stream);

void UnregisterWasiInstance(const Instance::Ptr& instance);
Copy link
Member

Choose a reason for hiding this comment

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

Can we make all these function symbols start with Wasi?

@tDwtp
Copy link
Contributor Author

tDwtp commented Oct 1, 2022

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.

@tDwtp tDwtp changed the title wasm-interp: Implement module loading wasm-interp: Add multi-module support Oct 14, 2022
@tDwtp
Copy link
Contributor Author

tDwtp commented Oct 14, 2022

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

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

#include "uvwasi.h"

#include <cstdio>
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Is the double & needed here?

return module_arg.substr(path_at);
}

static std::string FileNameOfPath(std::string path_name) {
Copy link
Member

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 from wabt/filenames.h?

Copy link
Contributor Author

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.

}

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

Choose a reason for hiding this comment

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

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@tDwtp tDwtp Oct 14, 2022

Choose a reason for hiding this comment

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

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.

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

// Unregister memory
wasiInstances.erase(instance.get());
UnregisterWasiInstance(instance);
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be renamed I think.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@tDwtp tDwtp Oct 14, 2022

Choose a reason for hiding this comment

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

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
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
Copy link
Contributor Author

tDwtp commented Oct 14, 2022

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants