Fix v8 validation error in -O0 builds #897
Conversation
…ust have the right type. ensure we generate it that way in asm2wasm, and keep it that way through refinalizations
|
As a followup, we should probably change wasm-validator to assert on this. However, doing so naively brings up errors, so let's leave that for later. |
There was a problem hiding this comment.
So this bug only affects the implicit function-scope block?
I guess i was confused about that last time I looked at that too, actually. In the binaryen IR it seems like the function body is not always a block (indicating that it could be implicit) but if it is a block, sometimes it is printed, and sometimes not. And sometimes it has a name, (and therefore can be targeted by breaks). Am I just confused? What are/should be the conventions for the function-level block (is it real or just a construct to make the IR cleaner? is it printed? does it have a name? does the function body have to be a block?)
| @@ -107,6 +107,10 @@ | |||
| cmd += ['-O0'] # test that -O0 does nothing | |||
| else: | |||
| cmd += ['-O'] | |||
| if 'PRINT_FULL' in asm: | |||
There was a problem hiding this comment.
If this is adding a class of tests that can be ran with BINARYEN_PRINT_FULL set, then the test that this PR adds should have a name besides just PRINT_FULL.asm, something like PRINT_FULL.unreachable_function_block_type.asm.js I'd think
There was a problem hiding this comment.
Yeah, I considered that, but instead I documented this in more detail in the test itself. i.e., we might have just one PRINT_FULL file with multiple small unit tests inside it eventually, with some text describing each one in the file. I prefer that because a filename isn't much room to go into detail. if i'm in the minority though i can change that.
There was a problem hiding this comment.
In that case why not have the test script only set this if asm == 'PRINT_FULL.asm.js'? That way the intent of multiple small tests in this one special file is documented by the code itself.
There was a problem hiding this comment.
Fair point, I wanted to keep my options open ;) In case we do want multiple files, this would mean we don't need to change anything.
Anyhow, I don't feel strongly about any of it. If you do, let me know what you'd prefer.
There was a problem hiding this comment.
Don't feel strongly enough about it to block
Actually a best-of-both-worlds thing would be to leave the PRINT_FULL substring check, and rename the test to PRINT_FULL_generic.asm.js or something. That way both are reasonably-well supported, and it's more clear that generic tests go there.
|
Yeah the validator should definitely match whatever the spec and/or browsers do. I think validation is pretty ad-hoc right now because of the history of the IR (and the ways it differs now from wasm?). Anyway for now we can just fix this. |
That seemed to be the case until I compiled bigger things ;) This fixes hello world, but there is a more complex validation error on
Binaryen IR has the function body as an expression - not a list. Similar to loop, if, etc. - the only thing with a list is block. We have an optimization to not print the block, if it's the toplevel of a function, loop or if, as the s-expression format allows them to have lists. PRINT_FULL overrides that, showing the full ast + types. |
|
This is obsoleted by #899. |
v8 changed the validation rules to be closer to the spec interpreter, and we had a bug where we emitted the toplevel block of a returning function with an
unreachabletype, when it should be the return type.