wasmpaser: Validate nested modules #30
Conversation
|
FWIW I've been wrestling a lot with the current design of the validator. I find it really difficult to work with the way it's set up, namely how there's one massive state enum and we just transition over that as we would a state machine. This is quite unwieldy and difficult to work with what would otherwise be pretty simple to work with I think. One major drawback of this implementation is that there's really no avenue for parallel module validation. That's a major design consideration with the section orderings, however, and is something that I think we should enable. I've also found, however, that there's a number of other drawbacks with the current validator and/or parser such as they can't accept incremental input (e.g. downloaded over the network). Additionally there's a lot of state being learned as part of nested modules which is going to have to be figured out again by consumers after validation. Ideally this would also be structured so consumers could persist information such as "what's the type of this function?" in a way that they don't have to figure out all the parent module relationships again. What I'm thinking, however, is basically a rewrite of the validator. While this isn't necessarily a huge undertaking it's large enough that I don't want to entangle it here or anything like that. I'm hoping that as this come up in |
|
Ok this ran on CI, but I'm not sure how to read the output myself.. |
|
Does that look as expected to you @yurydelendik ? |
The results are good, but it looks like it did not checkout "main" (see |
|
I see, so not much effect from this PR P.S. we need to fix ./compare-with-main.sh so it is getting valid "main" |
|
What do these numbers mean? Is that something executing in 0.4ns? |
|
Yeah 1ns runtimes typically means tests aren't running! |
This commit implements recursive validation of nested modules in wasmparser. Previously nested modules were simply skipped but now they're recursed into and actually checked. This commit also comes with a more complete implementation of handling `alias` directives. Internally this required quite a bit of refactoring. The validator now retains a stack of modules which are being validated and remembers parent-relationships between them. Indices into this stack are then used for recording type definitions. The type of a function/instance/module/etc can be defined the current module or any previous module on the stack. New helper functiosn were added to help resolve this new `Def` type to ensure it's handled correctly.
This was previously trying to validate all functions in the context of the top-level module, when instead it needed to validate each function within the right module.
|
Ok now it is saying I fixed another issue after this measurement was taken, though, which ensures that it actually checks out all submodules so we can include wabt/spec tests in the measurement. I don't doubt that this iteration is slower (bounds checking) but the benchmarking strategy is also somewhat flawed because the FWIW I'm not sure that we can do all that much better without |
I was planning to fade Parser and ValidatingParser away. Hopefully you are not thinking about refactoring this API. Shall we just deprecate them, and use readers + something instead? |
|
I don't have any changes in the works, but I've been thinking recently about how things might be refactored. I'm planning on waiting until bytecodealliance/rfcs#1 is settled though and going through that process for making a proposal (and going through a more formal proposal before doing code changes). To clarify, though, @yurydelendik do you feel it's ok to merge this as-is or would you prefer I look into the perf difference further? |
It is good to go. I'm mostly monitoring huge "it works benchmark" regression. |
This commit exposes the `Module::resolve` method which allows accessing the results of name resolution. The thinking here is that formats like wasm interface types will want to access the function indices of the core module but by name as well, so we can expose the results of name resolution so the interface types section can access the same results.
Merge with release wasm-tools-1.0.51
This commit implements recursive validation of nested modules in
wasmparser. Previously nested modules were simply skipped but now
they're recursed into and actually checked. This commit also comes with
a more complete implementation of handling
aliasdirectives.Internally this required quite a bit of refactoring. The validator now
retains a stack of modules which are being validated and remembers
parent-relationships between them. Indices into this stack are then used
for recording type definitions. The type of a
function/instance/module/etc can be defined the current module or any
previous module on the stack. New helper functiosn were added to help
resolve this new
Deftype to ensure it's handled correctly.