Stream: git-wasmtime

Topic: wasmtime / Issue #2637 Allow missing imports in WASM modu...

Wasmtime GitHub notifications bot (Feb 04 2021 at 13:54):

github-actions[bot] commented on Issue #2637:

Subscribe to Label Action

cc @peterhuene

<details>
This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

• peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Feb 04 2021 at 15:29):

alexcrichton commented on Issue #2637:

API-wise I personally feel like this would instead be a flag on Linker rather than a compile-time option, but the code is also so small here to support this I think it may be worthwhile exploring how we might be able to support this externally. Creating a Func::new which simply panics or returns a trap isn't so hard, and ideally clients that would specifically like to allow imports they don't allow could opt-in easily by implementing such behavior.

For example one option to implement this externally today is that instead of Linker::instantiate you'd use Linker::get manually on the module's imports, filling in missing ones as necessary. Put another way I think you could write a function that takes &Linker and &Module and instantiates the module with this behavior only using the public API of the wasmtime crate.

One worry I'd have with this as-is as well is that it assumes all missing imports are missing function imports, but it could be a missing import of any type and it's not entirely clear what you'd do for the other types of missing imports (e.g. a missing imported table for example).

Wasmtime GitHub notifications bot (Feb 04 2021 at 19:23):

AlexEne commented on Issue #2637:

Yes, it just allows functions to be missing, for other things it still has the behavior it had today, as I'm not sure what a "sane" alternative would be for missing non-functions.

I like your alternative, I wasn't aware it's possible. do you have an example where those APIs are used that I could start from?

Wasmtime GitHub notifications bot (Feb 04 2021 at 19:42):

alexcrichton commented on Issue #2637:

I believe a function like this should work for your purposes:

fn instantiate(linker: &Linker, module: &Module) -> Result<Instance> {
    let mut imports = Vec::new();
    for import in module.imports() {
        match linker.get(&import) {
            Some(value) => imports.push(value),
            None => match import.ty() {
                ExternType::Func(ty) => imports.push(
                    Func::new(linker.store(), ty, |_, _, _| {
                        Err(Trap::new("function not implemented"))
                    })
                    .into(),
                ),
                _ => anyhow::bail!("import not defined"),
            },
        }
    }
    Instance::new(linker.store(), module, &imports)
}

Wasmtime GitHub notifications bot (Feb 05 2021 at 19:09):

tschneidereit commented on Issue #2637:

I do feel that having a Linker flag for this would be good, perhaps called something like unstable_allow_missing_function_imports or so, to make the "functions only" part clear.

Additionally, it'd be great to also add this as an option --unstable-allow-missing-function-imports to the CLI. @AlexEne, do you think that's something you could add?

Wasmtime GitHub notifications bot (Feb 06 2021 at 05:26):

AlexEne commented on Issue #2637:

Yes, I can check how to add that to the CLI too. (and fix the name)

Wasmtime GitHub notifications bot (Feb 08 2021 at 11:38):

AlexEne commented on Issue #2637:

@tschneidereit on the command line option. I've added a unstable_allow_missing_function_imports in wasmtime/src/lib.rs in CommonOptions, and in the Config class from wasmtime/src/config.rs.

I've also added a boolean setting in the linker, but it's unclear to me where I should configure the linker to use that value from Config.

Wasmtime GitHub notifications bot (Feb 08 2021 at 15:19):

AlexEne edited a comment on Issue #2637:

@tschneidereit on the command line option. I've added a unstable_allow_missing_function_imports in wasmtime/src/lib.rs in CommonOptions, and in the Config class from wasmtime/src/config.rs.

I've also added a boolean setting in the linker, but it's unclear to me where I should configure the linker to use that value from Config.

@alexcrichton: On the manual specification of missing methods, the code does look promising, I did translate it to C as that's how i'm using wasmtime, so it will require some changes to expose the linker.get( methods through the c-api crate as it's not public now.
In adition to that, crafting the trap message from C is a bit clunky as wasm_trap_new requires the store as a parameter. This means that I have to bypass it by using a global static wasm_trap_t* as the store can't be captured in a C++ lambda in order to dynamically create the trap when the missing method is invoked from wasm.

Wasmtime GitHub notifications bot (Feb 08 2021 at 15:27):

AlexEne edited a comment on Issue #2637:

@tschneidereit on the command line option. I've added a unstable_allow_missing_function_imports in wasmtime/src/lib.rs in CommonOptions, and in the Config class from wasmtime/src/config.rs.

I've also added a boolean setting in the linker, but it's unclear to me where I should configure the linker to use that value from Config.

@alexcrichton: On the manual specification of missing methods, the code does look promising, I did translate it to C as that's how i'm using wasmtime, so it will require some changes to expose the linker.get( methods through the c-api crate as it's not public now. This also requires making ImportType public as now it's pub(crate).
In adition to that, crafting the trap message from C is a bit clunky as wasm_trap_new requires the store as a parameter. This means that I have to bypass it by using a global static wasm_trap_t* as the store can't be captured in a C++ lambda in order to dynamically create the trap when the missing method is invoked from wasm.

Wasmtime GitHub notifications bot (Feb 08 2021 at 15:45):

alexcrichton commented on Issue #2637:

I don't think that this needs to be in Config, at most I think it just needs a flag on Linker. @tschneidereit would you be ok with just a CLI flag, though? Since this is easy enough to implement externally and since this is in theory behavior that will get subsumed by wasm standards in the future, I'd prefer to avoid changing the Linker type if we can.

@AlexEne for the C API I think wasm_importtype_t already exists, and for acquiring the store you could either store that in a global variable or use the wasmtime_* APIs for functions which give you a wasmtime_caller_t which you can acquire the store from. We may not have the method in the C API yet to get the wasm_store_t from that but it's possible to implement.

Wasmtime GitHub notifications bot (Feb 08 2021 at 17:33):

AlexEne commented on Issue #2637:

Yes, wasm_importtype_t exists, the one that I mentioned was ImportType (the rust counterpart and parameter to linker.get( as that's currently defined as pub(crate) and can't be created in order to call the linker current method. It's not a big deal, I can shuffle things around so it's possible to create it from the c-api crate.

Wasmtime GitHub notifications bot (Mar 12 2021 at 13:32):

AlexEne commented on Issue #2637:

It seems that there isn't really an consensus on how to proceed with this, so I will close this PR for now, and will handle this in my mirror as my use-case can be solved by a patch of a couple of lines in the linker that makes this behavior default.

Once optional imports are implemented this would not be needed anyway.

Last updated: Nov 22 2024 at 16:03 UTC