feat: Linker #87
feat: Linker #87
Conversation
github-actions
bot
added
execution
validation
tests
priority-high
priority-medium
labels
|
Blocked on #88 |
… for linker Signed-off-by: George Cosma <[email protected]>
george-cosma
force-pushed
the
dev/george-linker
branch
3 times, most recently
from
737c1a5 to
9e29849
Compare
Codecov ReportAttention: Patch coverage is
|
| // TODO: what do we want to do if there is a missing import/export pair? Currently we fail the entire | ||
| // operation. Should it be a RuntimeError if said missing pair is called? |
There was a problem hiding this comment.
As per internal discussion, "better safe than sorry"
|
|
||
| // TODO: how do we handle the start function, if we don't have a LUT yet? |
There was a problem hiding this comment.
If a module has a start function, this function will error out if we attempt to run "start" if there are any unmet imports, even if they are not used in "start" itself.
There was a problem hiding this comment.
Running an example where we have an import and it's not met in wasm-interp leads to the same error: invalid import. I think this is the safe way to go about it.
(wasm-interp actually errors out even if we don't run any of the code:
(module
(import "console" "log" (func $log (param i32)))
(func $dummy
i32.const 0
drop
)
;; (start $dummy)
))
Signed-off-by: George Cosma <[email protected]>
george-cosma
force-pushed
the
dev/george-linker
branch
from
9e29849 to
bf9202b
Compare
george-cosma
requested review from
valexandru,
wucke13,
nerodesu017 and
florianhartung
| @@ -288,20 +357,30 @@ where | |||
| let (module_idx, func_idx) = | |||
| self.get_indicies(&function_ref.module_name, &function_ref.function_name)?; | |||
|
|
|||
| if module_idx != function_ref.module_index || func_idx != function_ref.function_index { | |||
| // TODO: should we return a different error here? | |||
| if module_idx != function_ref.module_index { | |||
There was a problem hiding this comment.
Maybe just one if statement here with both conditions combined?
There was a problem hiding this comment.
The reason I wanted to split them off was to be able to discern which reference is wrong - the module or the function. This was done to conform to the two errors: ModuleNotFound and FunctionNotFound. However, it would also be valid to say there should be a 3rd error type to indicate a stale or incorrect reference
There was a problem hiding this comment.
I see, that makes sense. I also agree on the 3rd error type for an incorrect reference.
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
This scheme can't handle cases where imports cascade. Let A, B, C be modules where A imports func B.f and rexports A.g, and C import A.g as C.h. When h is called, we need to recursively follow until a local func is hit (which we might not). we might need to check the func_idx is not imported itself here, and if it is, follow the other module without a recursive func call (stackless interpreter rule). We should also throw error if the (module_idx,func_idx) ends up the same as local_module_idx, local_func_idx, since that indicates a cyclic reference. But we can also make do with this too and leave it as an issue.
|
|
||
| let wasm_bytes = wat::parse_str(SIMPLE_IMPORT_ADDON).unwrap(); | ||
| let validation_info = validate(&wasm_bytes).expect("validation failed"); | ||
| instance.add_module("env", &validation_info); |
There was a problem hiding this comment.
This a bit nitpicky, but I am more in favor of an interface where if A depends on B's store values for instantiation and B depends on nothing, B is instantiated with a RuntimeInstance::new_named("B") and then we instantiate a with RuntimeInstance::new_named_with("A", instance_of_B/imports??) sort of thing, since instantiation of A might depend on already instantiated const values etc. in B (https://webassembly.github.io/spec/core/exec/modules.html#external-typing) here I think the spec necessitates that the imports are instantiated within their module store. For globals at least this is definitely required, since a global.get of an imported const global can be used in constant exprs during instantiation, and that itself needs to have been instantiated. I think in js module instantiation is also this way, but i am not sure. But this can stay too.
Pull Request Overview
This pull request adds a "Lookup table" linker. See #83 first.
Basically, after each time a new module is added we attempt to create a "lookup table". This lookup table translate the imported functions from each module to the exported local function in the already-existing modules.
Resumability is kept by passing all module-specific details (like the wasm binary, the readers, function types, store, etc.) to the interpreter_loop::run function. These details have been compiled into a structure called
ExecutionInfo.Also of note, I split the "FunctionInstance" structure into two variants - local and imported, for clarity sake. This comes with the needed modification to the validation process and store-creation process.
Testing Strategy
This pull request was tested by writing a new test, and tested against the existing tests.
Formatting
cargo fmtcargo checkcargo buildcargo docnix fmtGithub Issue
This pull request closes #83