Skip to content

Fix parsing error on array.new_fixed on non-array #7604

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 2 commits into from
May 16, 2025
Merged

Fix parsing error on array.new_fixed on non-array #7604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
10a6c73
fix
kripken May 16, 2025
2b89d33
fix
kripken May 16, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/wasm/wasm-ir-builder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2213,6 +2213,9 @@ Result<> IRBuilder::makeArrayNewElem(HeapType type, Name elem) {

Result<> IRBuilder::makeArrayNewFixed(HeapType type, uint32_t arity) {
ArrayNewFixed curr(wasm.allocator);
if (!type.isArray()) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this has to go here in IRBuilder? It doesn't look like the other allocation instructions have similar checks. I think we normally try to leave this for validation.
Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah, the simpler constructors don't have that... I copied this from makeArrayInitElem:

binaryen/src/wasm/wasm-ir-builder.cpp

Lines 2280 to 2282 in 2b89d33

if (!type.isArray()) {
return Err{"expected array type annotation on array.init_elem"};
}

Is that one also in the wrong place?
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, my guess is that we add these guards only when necessary to avoid assertion failures in ChildTyper. Is that what's happening here?
Copy link
Member Author

@kripken kripken May 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that is the case. Without this PR, the crash is from IRBuilder, long before the validator,

(gdb) where
..
#5  in wasm::HeapType::getArray (this=0x7ffff15c3440) at binaryen/src/wasm/wasm-type.cpp:932
#6  in wasm::ChildTyper<wasm::IRBuilder::ChildPopper::ConstraintCollector>::visitArrayNewFixed (this=0x7ffff15c3360, curr=0x7ffff168c620) at src/ir/child-typer.h:947
#7  in wasm::Visitor<wasm::IRBuilder::ChildPopper::ConstraintCollector, void>::visit (this=0x7ffff15c3360, curr=0x7ffff168c620) at src/wasm-delegations.def:89
#8  in wasm::IRBuilder::ChildPopper::visitExpression (this=0x7ffff1461230, expr=0x7ffff168c620)
    at binaryen/src/wasm/wasm-ir-builder.cpp:579
#9  in wasm::UnifiedExpressionVisitor<wasm::IRBuilder::ChildPopper, wasm::Result<wasm::Ok> >::visitArrayNewFixed (this=0x7ffff1461230, curr=0x7ffff168c620)
    at src/wasm-delegations.def:89
#10 in wasm::Visitor<wasm::IRBuilder::ChildPopper, wasm::Result<wasm::Ok> >::visit (this=0x7ffff1461230, curr=0x7ffff168c620) at src/wasm-delegations.def:89
#11 in wasm::IRBuilder::visitExpression (this=0x7ffff1922130, curr=0x7ffff168c620) at binaryen/src/wasm/wasm-ir-builder.cpp:755
#12 in wasm::UnifiedExpressionVisitor<wasm::IRBuilder, wasm::Result<wasm::Ok> >::visitArrayNewFixed (this=0x7ffff1922130, curr=0x7ffff168c620)
    at src/wasm-delegations.def:89
#13 in wasm::IRBuilder::makeArrayNewFixed (this=0x7ffff1922130, type=..., arity=1) at binaryen/src/wasm/wasm-ir-builder.cpp:2218
..

ChildTyper assumes the IR is valid enough for getArray to work, so we must check early.
return Err{"expected array type annotation on array.new_fixed"};
}
curr.type = Type(type, NonNullable);
curr.values.resize(arity);
CHECK_ERR(visitArrayNewFixed(&curr));
Expand Down
12 changes: 12 additions & 0 deletions test/lit/validation/array-new-fixed.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
;; RUN: not wasm-opt -all %s 2>&1 | filecheck %s

;; CHECK: expected array type annotation on array.new_fixed

(module
(type $struct (struct (field anyref)))

;; The type here is a struct, not an array, so we error.
(global $struct (ref $struct) (array.new_fixed $struct 1
(i32.const 64)
))
)
Loading