Skip to content

Save local set data in EndTryTable #2673

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 1 commit into from
Nov 24, 2025
Merged

Save local set data in EndTryTable #2673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 1 addition & 2 deletions src/shared-validator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1422,7 +1422,6 @@ Result SharedValidator::BeginTryTable(const Location& loc, Type sig_type) {
result |= CheckBlockSignature(loc, Opcode::TryTable, sig_type, &param_types,
&result_types);
result |= typechecker_.BeginTryTable(param_types);
SaveLocalRefs();
return result;
}

Expand All @@ -1447,8 +1446,8 @@ Result SharedValidator::EndTryTable(const Location& loc, Type sig_type) {
TypeVector param_types, result_types;
result |= CheckBlockSignature(loc, Opcode::TryTable, sig_type, &param_types,
&result_types);
RestoreLocalRefs(result);
result |= typechecker_.EndTryTable(param_types, result_types);
SaveLocalRefs();
return result;
}

Expand Down
32 changes: 32 additions & 0 deletions test/parse/bad-refs-in-trytable.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
;;; TOOL: wat2wasm
;;; ARGS: --enable-function-references --enable-exceptions
;;; ERROR: 1
(module
(func (param (ref func))
(local (ref func) (ref func))
local.get 0
local.set 1
try_table
try_table
local.get 0
local.set 2
end
local.get 1
local.set 0
local.get 2
local.set 0
end
local.get 1
local.set 0
local.get 2
local.set 0
)
)
(;; STDERR ;;;
out/test/parse/bad-refs-in-trytable.txt:16:17: error: uninitialized local reference
local.get 2
^
out/test/parse/bad-refs-in-trytable.txt:21:15: error: uninitialized local reference
Copy link
Member

@sbc100 sbc100 Nov 24, 2025

Choose a reason for hiding this comment

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

Is this is test that is missing from the upstream repo? Perhaps we should file an issue there, with the hope that can one day remove this downstream test?

Copy link
Contributor Author

@zherczeg zherczeg Nov 24, 2025

Choose a reason for hiding this comment

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

I am not sure I understand this question. I have created this test myself. The test checks that the "is set" bitset is correctly working. My code was wrong, I saved the bitset in a wrong place. I misunderstood the API sequence order for try_tables.

Copy link
Contributor Author

@zherczeg zherczeg Nov 24, 2025

Choose a reason for hiding this comment

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

Ah, you might refer to the failure. That is intentional, the WebAssembly set-before-use check is very simple.

The spec test interpreter also yields this error (although stops at the first error):
test.wast:13.17-13.18: validation error: uninitialized local

Copy link
Member

@sbc100 sbc100 Nov 24, 2025

Choose a reason for hiding this comment

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

My question is basically this: If this is an interesting test case, why can't it be (or shouldn't it be) upstream in the spec repo. I guess this is question we should be asking of all of our local/downstream tests.

Copy link
Member

@sbc100 sbc100 Nov 24, 2025

Choose a reason for hiding this comment

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

In any case this change LGTM

Copy link
Contributor Author

@zherczeg zherczeg Nov 24, 2025

Choose a reason for hiding this comment

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

If they are interested, why not. Anyway, it is possible that a similar test is already present in the spec test repo. The GC/function-references proposal don't have such test, since (reworked) exceptions is another proposal, but the full WebAssembly 3.0 test system may have.

local.get 2
^
;;; STDERR ;;)
26 changes: 23 additions & 3 deletions test/regress/regress-2670.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,17 @@
;; save/restore operations was missing for try tables

(module
(func (local (ref func)))
(tag $e0)

(func (local (ref func) (ref func) (ref func)))
(func
(local (ref func) (ref func))
try_table
end)
(func
(local (ref func))
try_table (catch $e0 0) (catch_all 0)
end)
(func
unreachable
try_table
Expand All @@ -16,9 +26,19 @@
(module
(type $t0 (func))
(func $f0 (type $t0)
(local $l0 (ref func)))
(local $l0 (ref func)) (local $l1 (ref func)) (local $l2 (ref func)))
(func $f1 (type $t0)
(local $l0 (ref func)) (local $l1 (ref func))
(try_table $T0
))
(func $f2 (type $t0)
(local $l0 (ref func))
(try_table $T0
(catch $e0 0 (;@0;))
(catch_all 0 (;@0;))))
(func $f3 (type $t0)
(unreachable)
(try_table $T0
)))
))
(tag $e0 (type $t0)))
;;; STDOUT ;;)
Loading