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

Linker does not properly define imports (C API) #2211

Closed
7Hazard opened this issue Sep 20, 2020 · 5 comments · Fixed by #2579
Closed

Linker does not properly define imports (C API) #2211

7Hazard opened this issue Sep 20, 2020 · 5 comments · Fixed by #2579
Assignees
Labels
bug Incorrect behavior in the current implementation that needs fixing

Comments

@7Hazard
Copy link

7Hazard commented Sep 20, 2020

Using the C API, defining imports with the Linker do not work properly. When attempting to instantiate the module in question, it errors with the following:
unknown import: `env::Test` has not been defined

This is how the module is attempted to be instantiated:

wasm_trap_t* trap = nullptr;
wasm_instance_t* instance = nullptr;

wasm_engine_t* engine = wasm_engine_new();
wasm_store_t* store = wasm_store_new(engine);
wasmtime_linker_t* linker = wasmtime_linker_new(store);

// Read wasm file
wasm_byte_vec_t wasm;
std::ifstream file("wasmtest.wasm", std::ios::binary | std::ios::ate);
if(!file.good()) {
    std::cout << "File wasmtest.wasm bad" << std::endl;
    return 1;
}
auto filesize = file.tellg();
file.seekg(0, std::ios::beg);
wasm_byte_vec_new_uninitialized(&wasm, filesize);
if(!file.read(wasm.data, filesize))
{
    std::cout << "Could not read file" << std::endl;
    return 1;
}

// Compile module
wasm_module_t* module = nullptr;
error = wasmtime_module_new(engine, &wasm, &module);
if (error != nullptr)
{
    logWasmError("Failed to compile module", error, nullptr);
    return 1;
}
wasm_byte_vec_delete(&wasm);

// MAKE IMPORTS
{
    auto import = [](const wasm_val_t args[], wasm_val_t results[])->wasm_trap_t*{
        printf("Hello world");
        return nullptr;
    };
    wasm_valtype_vec_t types;
    wasm_valtype_vec_new_uninitialized(&types, 0);
    wasm_functype_t *type = wasm_functype_new(&types, &types);
    auto function = wasm_func_new(store, type, import);
    auto extern_ = wasm_func_as_extern(function);
    
    wasm_name_t module;
    wasm_name_new_from_string(&module, "env");
    wasm_name_t func;
    wasm_name_new_from_string(&func, "Test");
    wasmtime_linker_define(linker, &module, &func, extern_);
    wasm_name_delete(&func);
}

// INSTANTIATE MODULE
error = wasmtime_linker_instantiate(linker, module, &instance, &trap);
if (error || trap)
{
    logWasmError("Failed to instantiate module", error, trap);
    return 1;
}

This is the module that is being attempted to be loaded (compiled with clang):

extern "C" void Test();

extern "C" void main() {
    Test();
}

The imports in the wat looks like this:

(type $t0 (func))
(import "env" "Test" (func $Test (type $t0)))

This was tested on Windows 10, using the C API build from https://github.com/bytecodealliance/wasmtime/releases/tag/v0.19.0.

@7Hazard 7Hazard added the bug Incorrect behavior in the current implementation that needs fixing label Sep 20, 2020
@alexcrichton
Copy link
Member

Thanks for the report! The issue here is somewhat subtle and unfortunate. The wasm_name_new_from_string function will have the length of the name including the terminating nul character, but the wasmtime_linker_define API does not expect the nul character to be included, so the name you're defining is actually ("env\u{0}", "Test\u{0}"), which isn't what you want here.

@7Hazard
Copy link
Author

7Hazard commented Sep 22, 2020

I see, indeed it was very subtle. However wouldn't this be a defect for wasm_name_new_from_string as an API function? I'm not sure if there are other purposes for wasm_name_new_from_string and wasm_name_t.

@alexcrichton
Copy link
Member

I believe the upstream API was adjusted in WebAssembly/wasm-c-api#151, and we haven't pulled in that update yet.

@Dakror
Copy link

Dakror commented Jan 12, 2021

Could this be addressed? The fix of pulling upstream should be trivial, no?

@peterhuene peterhuene self-assigned this Jan 13, 2021
@peterhuene
Copy link
Member

We should be able to update the Wasmtime C API to fix this now. I'll try to get a fix up tomorrow.

You should be able to work around the issue using wasm_name_new with the current implementation, if needed.

peterhuene added a commit to peterhuene/wasmtime that referenced this issue Jan 13, 2021
This commit updates the WebAssembly C API submodule (for `wasm.h`) to the
latest commit out of master.

This fixes the behavior of `wasm_name_new_from_string` such that it no longer
copies the null character into the name, which caused unexpected failures when
using the Wasmtime linker as imports wouldn't resolve when the null was
present.

Along with this change were breaking changes to `wasm_func_call`, the host
callback signatures, and `wasm_instance_new` to take a vector type instead of a
pointer to an unsized array.

As a result, Wasmtime language bindings based on the C API will need to be
updated once this change is pulled in.

Fixes bytecodealliance#2211.
Fixes bytecodealliance#2131.
alexcrichton pushed a commit that referenced this issue Jan 14, 2021
* Update WebAssembly C API submodule to latest commit.

This commit updates the WebAssembly C API submodule (for `wasm.h`) to the
latest commit out of master.

This fixes the behavior of `wasm_name_new_from_string` such that it no longer
copies the null character into the name, which caused unexpected failures when
using the Wasmtime linker as imports wouldn't resolve when the null was
present.

Along with this change were breaking changes to `wasm_func_call`, the host
callback signatures, and `wasm_instance_new` to take a vector type instead of a
pointer to an unsized array.

As a result, Wasmtime language bindings based on the C API will need to be
updated once this change is pulled in.

Fixes #2211.
Fixes #2131.

* Update Doxygen comments for wasm.h changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior in the current implementation that needs fixing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants