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>
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 aFunc::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 useLinker::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 thewasmtime
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).
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?
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) }
tschneidereit commented on Issue #2637:
I do feel that having a
Linker
flag for this would be good, perhaps called something likeunstable_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?
AlexEne commented on Issue #2637:
Yes, I can check how to add that to the CLI too. (and fix the name)
AlexEne commented on Issue #2637:
@tschneidereit on the command line option. I've added a
unstable_allow_missing_function_imports
inwasmtime/src/lib.rs
inCommonOptions
, and in theConfig
class fromwasmtime/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
.
AlexEne edited a comment on Issue #2637:
@tschneidereit on the command line option. I've added a
unstable_allow_missing_function_imports
inwasmtime/src/lib.rs
inCommonOptions
, and in theConfig
class fromwasmtime/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 thelinker.get(
methods through thec-api
crate as it's not public now.
In adition to that, crafting the trap message from C is a bit clunky aswasm_trap_new
requires thestore
as a parameter. This means that I have to bypass it by using a globalstatic 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.
AlexEne edited a comment on Issue #2637:
@tschneidereit on the command line option. I've added a
unstable_allow_missing_function_imports
inwasmtime/src/lib.rs
inCommonOptions
, and in theConfig
class fromwasmtime/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 thelinker.get(
methods through thec-api
crate as it's not public now. This also requires makingImportType
public as now it'spub(crate)
.
In adition to that, crafting the trap message from C is a bit clunky aswasm_trap_new
requires thestore
as a parameter. This means that I have to bypass it by using a globalstatic 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.
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 onLinker
. @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 theLinker
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 thewasmtime_*
APIs for functions which give you awasmtime_caller_t
which you can acquire the store from. We may not have the method in the C API yet to get thewasm_store_t
from that but it's possible to implement.
AlexEne commented on Issue #2637:
Yes,
wasm_importtype_t
exists, the one that I mentioned wasImportType
(the rust counterpart and parameter tolinker.get(
as that's currently defined aspub(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 thec-api
crate.
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: Dec 23 2024 at 12:05 UTC