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

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

Closed
tDwtp opened this issue Sep 22, 2022 · 13 comments
Closed

Comments

@tDwtp
Copy link
Contributor

tDwtp commented Sep 22, 2022

I would like to make imports simpler for wasm testing with the wasm-interp tool.
Nothing fancy just something like
-m, --module=[NAME:]PATH
use PATH as a wasm module and provide all exports under the name NAME (filename if not present) for imports

This way you could load any wasm-modules or possibly native libraries and expose all names to others. testing wasm modules would be a lot simpler.

Adding the argument to the parser is fine, literally a oneliner.
However, I am confused on how to link exports from one module to the imports of others. I checked BindImports from src/tools/wasm-interp.cc and WasiBindImports from src/interp/interp-wasm.cc and I think I get how to add them to the imports-list, but how do I get the function for pushing onto the imports?

@keithw
Copy link
Member

keithw commented Sep 23, 2022

However, I am confused on how to link exports from one module to the imports of others.

Here's how the spectest-interp runner does it: https://github.com/WebAssembly/wabt/blob/main/src/tools/spectest-interp.cc#L1455-L1514

@keithw keithw closed this as completed Sep 26, 2022
@sbc100
Copy link
Member

sbc100 commented Sep 28, 2022

I can see an argument for wasm-interp itself being able to run more than one module at a time though? Should we leave this issue open in case somebody wants to add this feature?

@tDwtp
Copy link
Contributor Author

tDwtp commented Sep 28, 2022

I am working on it... I just had to work on weekend and have free today.

Also: As there can (potentially) be multiple memory entries. I would like to add the behaviour, that if a memory is exported using the name of the module it is importing, that is linked to the "memory" export of the imported module. or shoudl I just join by name? (I am still figuring out things..)

@sbc100
Copy link
Member

sbc100 commented Sep 28, 2022

Linking imports and exports should always be by name, and should work the same way for all types of exports.

@keithw
Copy link
Member

keithw commented Sep 28, 2022

Happy to reopen. :-) Agreed this would be a useful feature.

@keithw keithw reopened this Sep 28, 2022
@tDwtp
Copy link
Contributor Author

tDwtp commented Sep 28, 2022

Is there a chat, IRC or Discord or whatever... I am stuck.

@tDwtp
Copy link
Contributor Author

tDwtp commented Sep 28, 2022

To be more specific with my problem:
It compiles fine. It even sets the exports, but they dont seem to be loaded properly.
I basically do the same as described by these 3 functions:
https://github.com/WebAssembly/wabt/blob/main/src/tools/spectest-interp.cc#L1455-L1483
I just added the host.print exception.

Here is the loop over the module-arguments:

  std::vector<Module::Ptr> modules_loaded(s_modules.size());
  ExportMap load_map;
  for (auto& import_module : s_modules) {
    std::string module_path = GetPathName(import_module);

    Module::Ptr load_module;
    result = ReadModule(module_path.c_str(), &errors, &load_module);
    if (!Succeeded(result)) {
      FormatErrorsToFile(errors, Location::Type::Binary);
      return result;
    }

    RefVec load_imports;
    BindImports(load_module, load_imports);

    Instance::Ptr load_instance;
    CHECK_RESULT(InstantiateModule(load_imports, load_module, &load_instance));

    std::string reg_name = GetRegistryName(import_module, load_module);
    PopulateExports(load_instance, load_module, load_map);
    s_registry[reg_name] = std::move(load_map);

    modules_loaded.push_back(load_module);
  }

The error is a full on popup (crash) window titled Microsoft Visual C++ Runtime Library.

Debug Assertion Failed!

Program:
S:\Workspace\GitHub\wabt\build\Debug\wasm-interp.exe
File: C:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\VC\Tools\MSVC\14.29.30133\include\vector
Line: 1576

Expression: vector subscript out of range

For more information [...] {read the docs blah}

(pseudo-)code in [...]\include\vector boils down to:

const _SomeType operator[](const size_t pos) const noexcept {
    auto& data = {void*last; void*first};
    debug_assert(pos < static_cast<size_type>(data.last - data.fisrt));
    return data.first[pos];
}

@tDwtp tDwtp closed this as not planned Won't fix, can't repro, duplicate, stale Sep 28, 2022
@tDwtp
Copy link
Contributor Author

tDwtp commented Sep 28, 2022

I miss clicked sry

@tDwtp tDwtp reopened this Sep 28, 2022
@keithw
Copy link
Member

keithw commented Sep 28, 2022

Hmm, this code never seems to call GetImports or PopulateImports. In general, if you're putting each module's exports into a registry, you'll probably want to retrieve the imports from the same place when you instantiate the next module.

@tDwtp
Copy link
Contributor Author

tDwtp commented Sep 28, 2022

I do call PopulateImports: BindImports is the same as PopulateImports, with the host.print exception. They are the same, so I did not bother and just added the one missing line.
It would give errors error initializing module: invalid import "module.func"

BindImports now looks like:

static void BindImports(const Module::Ptr& module, RefVec& imports) {
  for (auto&& import : module->desc().imports) {
    if (IsHostPrint(import)) {
      imports.push_back(GenerateHostPrint(import));
      continue;
    }

    // By default, just push an null reference. This won't resolve, and
    // instantiation will fail.

    Extern::Ptr extern_ = GetImport(import.type.module, import.type.name);
    imports.push_back(extern_ ? extern_.ref() : Ref::Null);
  }
}

as you can see it is also more abstract. IsHostPrint, does the whole check. GenerateHostPrint should explain itself. (however it is not touched).
dummy export/imports are still present via IsHostPrint.

@tDwtp
Copy link
Contributor Author

tDwtp commented Sep 28, 2022

okay... i checked --run-all-exports and it works? I just cant find out why it errors out.
--dummy-import-func also works as expected.
I will push it to my branch module-loading in a sec.

EDIT: All you need is:
https://github.com/tDwtp/wabt/blob/module-loading/src/tools/wasm-interp.cc

@tDwtp
Copy link
Contributor Author

tDwtp commented Sep 28, 2022

The error happens on exit.
I can show this with a system("pause"); right before the return in ProgramMain like this:

  wabt::Result result = ReadAndRunModule(s_infile);
  system("pause");
  return result != wabt::Result::Ok;

I am really confused. Debugger does not help.

UPDATE:
the line map[export_type.name] = export_; for storing causes it to fail.
if i do not register the exports, everything is fine.

@tDwtp
Copy link
Contributor Author

tDwtp commented Sep 28, 2022

Found the error. It was a noob mistake. (I used std::move and forgot to clear the maps/registry and entries)
Pull request #2009.

@tDwtp tDwtp closed this as completed Sep 28, 2022
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

No branches or pull requests

3 participants