Do not emit recursion groups without GC enabled #4738
Conversation
We emit nominal types as a single large recursion group, but this produces invalid modules when --nominal or --hybrid was used without GC enabled. Fix the bug by always emitting types as though they were structural (i.e. without recursion groups) when GC is not enabled.
|
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
| @@ -242,7 +247,7 @@ void WasmBinaryWriter::writeTypes() { | |||
| BYN_TRACE("== writeTypes\n"); | |||
| auto start = startSection(BinaryConsts::Section::Type); | |||
| o << U32LEB(numGroups); | |||
| if (getTypeSystem() == TypeSystem::Nominal) { | |||
| if (typeSystem == TypeSystem::Nominal) { | |||
| // The nominal recursion group contains every type. | |||
| o << S32LEB(BinaryConsts::EncodedType::Rec) | |||
| << U32LEB(indexedTypes.types.size()); | |||
There was a problem hiding this comment.
Don't you need to do something to avoid new line 265 from being reached? Or is that handled by getRecGroup always returning the same?
There was a problem hiding this comment.
For --nominal, it all works out because the rec groups all have size 1. For --hybrid it all works because the MVP function types will also have rec group size 1, unless the input had rec groups. I don't think we have any validation that rec groups are only allowed when GC is enabled, but that seems like a separate problem.
| ;; RUN: wasm-opt %s --nominal --disable-gc -g -o %t.wasm | ||
|
|
||
| ;; We should not get any recursion groups even though we used --nominal | ||
| ;; RUN: wasm-opt %t.wasm --hybrid -all -S -o - | filecheck %s |
There was a problem hiding this comment.
| ;; RUN: wasm-opt %t.wasm --hybrid -all -S -o - | filecheck %s | |
| ;; RUN: wasm-opt %t.wasm -S -o - | filecheck %s |
I think it's better to test non-hybrid, and without -all, so that we are testing the default mode. That way this would test emitting with nominal and reading with no flags, which is what the fuzzer does. And the last step is closest to what users would do. The test then verifies we can load without erroring.
There was a problem hiding this comment.
The --hybrid is load-bearing here. Without it, we would just ignore any rec groups in the binary and the test would pass even with the bug present. -all similarly might be necessary to allow any rec groups in the binary to reach the output so that the test fails correctly. I'll mention this in the comment.
There was a problem hiding this comment.
Hmm, but it errors if rec groups are present but gc is disabled - that's the original error that led to this PR? I just want to make sure we have testing for that specifically. How about testing both paths?
There was a problem hiding this comment.
IIUC, It's wasm2c that errors in the presence of recursion groups, not binaryen.
There was a problem hiding this comment.
Binaryen errors too:
(module
(func $foo)
)$ wasm-opt -all --nominal a.wat -o a.wasm
warning: no passes specified, not doing any work
$ bin/wasm-opt a.wasm
[parse exception: Recursion groups not allowed with equirecursive typing (at 0:13)]
Fatal: error parsing wasm
There was a problem hiding this comment.
Ok, I added another run line that tests that we don't get a failure loading the module with the default flags.
We emit nominal types as a single large recursion group, but this produces
invalid modules when --nominal or --hybrid was used without GC enabled. Fix the
bug by always emitting types as though they were structural (i.e. without
recursion groups) when GC is not enabled.
Fixes #4723.