WebAssembly
Validate pops while ignoring nameless blocks?
@aheejin The fuzzer found another issue on pops in blocks (see test/lit/passes/gto-removals.wast in this PR), so I started to think that we might want a more general solution here. This draft PR changes validation so that nameless blocks, which vanish in the binary format, are ignored for validation. So makeSequence() etc. will never break pop validation.
This seems to work on the test suite. Do you remember if we considered this before? Is there a problem I am missing?
I think this makes sense. I don't think we've considered something like this. (Also we need to fix the new parser to handle this too, because currently it rejects parsing if a pop is nested) We omit all unnamed blocks, even if they are typed, right?
But I wonder why was this a problem before, for example, in #4237..? The example in #4237 also an unnamed block, but apparently it caused a validation error in the engine.
(Also we need to fix the new parser to handle this too, because currently it rejects parsing if a pop is nested)
Good catch, the fuzzer found that bug overnight :smile:
About the testcase in #4237, looks like it's an unnamed block so we should accept it as valid (rather than fix it up), if we go with this approach. But I'm not sure if I understood your question?
#4237 seems to say the binary was rejected at compile time from the engine. Does that mean we didn't omit the unnamed blocks in binary then? If we did why would the engine have rejected it?
I see now, sorry for misunderstanding before. Looks like that #4237 is from 2021, and we only added the guarantee of removing such blocks in 2022: the work was in #4943 and related PRs, and we documented the dependency on it when non-nullable local validation landed in #4959
@tlively I could use some advice on how to fix the parser here. This PR requires it to accept code like this:
https://github.com/WebAssembly/binaryen/pull/6950/files#diff-9d8e4085cec9051d98d5f9c2d98eb679d3f5ada3a6fb8800725023595524cb1cR10
In the last commit I believe I fixed up makePop to handle nested scopes like that, but it still errors on parsing, error: popping from empty stack. I added some debug logging (based on IR_BUILDER_DEBUG) and I see this, at the end of the block scope (IRBuilder::finishScope):
Scope stack in function $0:
scope func 0:
scope catch:
pop i32
scope block:
IIUC, the pop is in the catch, not in the block. I suspect that is because of
https://github.com/WebAssembly/binaryen/blob/b381a8c82030c2be3d1605d6f2854098f039f617/src/wasm/wasm-ir-builder.cpp#L868-L872
Does that code need to be changed to put the pop in the right place? I'm not sure how though.
Perhaps another option instead of this PR is, since this is in old EH, we can ignore the "pop must be in a catch" parser error in the fuzzer..?
Hmm, I just ran into a situation where IRBuilder already generates a pop in a block.
(module
(tag (param i32))
(func
try
catch 0
i32.const 3
drop
drop
end
)
)
Generates this IR:
(try
(do
(nop)
)
(catch $tag$0
(drop
(block (result i32)
(local.set 0)
(pop i32)
)
(drop
(i32.const 3)
)
(local.get $0)
)
)
)
)
I found this due to test/lit/binary/stack-eh-legacy.test when updating the binary parser to use IRBuilder.
The easiest path forward is probably to figure out how to land this PR without causing more problems. The two solutions I see are to do the thing where we reach up the stack and move the pop when parsing a pop in a block or to print nameless blocks as comments so text round tripping newly valid IR does not require the parser to parse a pop in a block.
@tlively Oh, I was thinking we didn't want to land this? Are you saying it would help the IRBuilder work?
For that code sample, running the EH fixups helper should fix things?
@tlively Oh, I was thinking we didn't want to land this? Are you saying it would help the IRBuilder work?
It would fix validation errors that you can get out of IRBuilder today, yes.
For that code sample, running the EH fixups helper should fix things?
Yes, but we probably don't want to do that every time we parse a module, right?
Yes, but we probably don't want to do that every time we parse a module, right?
Yeah, it's the same issue as with passes. We prefer not to run the EH fixups, so we try to run them only when we detect they might be needed, specifically when we add a block around a pop. Some passes have good ways to detect that. If IRBuilder can do so then that might be simpler than this PR, which changes what we accept as valid input.