Skip to content

Do not emit recursion groups without GC enabled #4738

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 18, 2022
Merged

Do not emit recursion groups without GC enabled #4738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5fc4882
Do not emit recursion groups without GC enabled
tlively Jun 17, 2022
96508b9
add missing test
tlively Jun 17, 2022
94316ba
add missing line terminator
tlively Jun 17, 2022
0bd705c
update comment
tlively Jun 17, 2022
2a0fe10
Add test with default flags
tlively Jun 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/wasm/wasm-binary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,12 @@ void WasmBinaryWriter::writeTypes() {
// the type section. With nominal typing there is always one group and with
// equirecursive typing there is one group per type.
size_t numGroups = 0;
switch (getTypeSystem()) {
// MVP types are structural and do not use recursion groups.
TypeSystem typeSystem = getTypeSystem();
if (!wasm->features.hasGC()) {
typeSystem = TypeSystem::Equirecursive;
}
switch (typeSystem) {
case TypeSystem::Equirecursive:
numGroups = indexedTypes.types.size();
break;
Expand All @@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
20 changes: 20 additions & 0 deletions test/lit/nominal-no-gc.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
;; Write the module with --nominal but without GC
;; RUN: wasm-opt %s --nominal --disable-gc -g -o %t.wasm

;; We should not get any recursion groups even though we used --nominal. We use
;; --hybrid -all here to make sure that any rec groups from the binary will
;; actually show up in the output and cause the test to fail.
;; RUN: wasm-opt %t.wasm --hybrid -all -S -o - | filecheck %s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
;; 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, It's wasm2c that errors in the presence of recursion groups, not binaryen.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I added another run line that tests that we don't get a failure loading the module with the default flags.


;; Also check that we don't get a failure with the default configuration.
;; RUN: wasm-opt %t.wasm

;; CHECK-NOT: rec

(module
(type $foo (func))
(type $bar (func))

(func $f1 (type $foo))
(func $f2 (type $bar))
)