[EH] Add validation for new instructions #6185

aheejin · 2023-12-19T19:17:30Z

This adds validation for the new EH instructions (try_table and throw_ref):
https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/Exceptions.md

This also adds a spec test for checking invalid modules. We cannot check the executions yet because we don't have the interpreter implementation. The new test file also contains tests for the existing throw, because this is meant to replace the old spec test someday.

This adds validation for the new EH instructions (`try_table` and `throw_ref`): https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/Exceptions.md This also adds a spec test for checking invalid modules. We cannot check the executions yet because we don't have the interpreter implementation. The new test file also contains tests for the existing `throw`, because this is meant to replace the old spec test someday.

src/wasm/wasm-validator.cpp

tlively · 2023-12-19T19:54:37Z

src/wasm/wasm-validator.cpp

+  if (curr->type != Type::unreachable) {
+    shouldBeSubType(curr->body->type,


This can be combined with the else case into a single shouldBeSubType, since the only subtype of Type::unreachable is Type::unreachable.

tlively · 2023-12-19T21:09:28Z

src/wasm/wasm-validator.cpp

+      if (shouldBeTrue(sentType.size() == 1, curr, "")) {
+        shouldBeEqual(sentType,
+                      Type(Type::none),


What is happening here? sentType cannot both have size 1 and be equal to Type::none, since Type::none has size 0.

So in case the sent type is none, the sentTypes will look like {Type(Type::none)}, which is of size 1, no?

To make it contain the sent types that can be used directly at branch-utils.h, in case there is no sent values, we add a single Type::none:

binaryen/src/wasm/wasm.cpp

Line 927 in eb5666e

curr->sentTypes.push_back(sentType.empty() ? Type::none : Type(sentType));

Usage in branch-utils.h

binaryen/src/ir/branch-utils.h

Lines 78 to 84 in eb5666e

} else if (auto* tt = expr->dynCast<TryTable>()) {

for (Index i = 0; i < tt->catchTags.size(); i++) {

auto dest = tt->catchDests[i];

if (dest == name) {

func(name, tt->sentTypes[i]);

}

}

We can make it {} and branch-utils.h's operateOnScopeNameUsesAndSentTypes to return Type::none in case there is an empty type, but that adds one more if and operateOnScopeNameUsesAndSentTypes runs more times than the routine popuating sentTypes when reading a binary.

Sorry, nevermind all above. I didn't know (Type::none).size() returns 0. Will fix.

tlively · 2023-12-19T21:17:30Z

src/wasm/wasm-validator.cpp

+      shouldBeEqual(
+        tagType[j], sentType[j], curr, "invalid catch sent type information");
+    }
+    if (curr->catchRefs[i]) {


It would be good to check that the sizes match when this is false as well. Alternatively, when there is no extra exnref, you can just check tagType == sentType.

This was actually incorrect because even if there is no values to send, sentType will contain Type::none, which is size 1... And this wasn't crashing because I was doing the early exit (https://github.com/WebAssembly/binaryen/pull/6185/files#diff-2149be365b66a82038f1d3fa8f9fb1b4dcd40ab535c986f4bf0a4c37e669a1ceR2481-R2483). Will fix that and check both cases.

Nevermind the comment. Didn't know (Type::none).size() returns 0...

Ended up rewriting much of the loop, but I think I an now checking both cases. PTAL

Co-authored-by: Alon Zakai <[email protected]>

tlively

Nice, LGTM!

tlively · 2023-12-20T07:55:26Z

src/wasm/wasm-validator.cpp

+    Name tagName = curr->catchTags[i];
+    if (!tagName) { // catch_all or catch_all_ref
+      tagTypeSize = 0;
+


nit: I think this extra newline looks a little odd.

This adds validation for the new EH instructions (`try_table` and `throw_ref`): https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/Exceptions.md This also adds a spec test for checking invalid modules. We cannot check the executions yet because we don't have the interpreter implementation. The new test file also contains tests for the existing `throw`, because this is meant to replace the old spec test someday.

aheejin requested review from kripken and tlively December 19, 2023 19:17

kripken approved these changes Dec 19, 2023

View reviewed changes

src/wasm/wasm-validator.cpp Outdated Show resolved Hide resolved

src/wasm/wasm-validator.cpp Outdated Show resolved Hide resolved

tlively reviewed Dec 19, 2023

View reviewed changes

aheejin and others added 2 commits December 19, 2023 16:25

Update src/wasm/wasm-validator.cpp

9b67a70

Co-authored-by: Alon Zakai <[email protected]>

Address comments

0bfa896

tlively approved these changes Dec 20, 2023

View reviewed changes

Update wasm-validator.cpp

f9c493e

aheejin merged commit 11e3af0 into WebAssembly:main Dec 20, 2023
14 checks passed

aheejin deleted the new_eh_validation branch December 20, 2023 18:48

gkdn mentioned this pull request Aug 31, 2024

stringconsts gkdn/binaryen#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EH] Add validation for new instructions #6185

[EH] Add validation for new instructions #6185

aheejin commented Dec 19, 2023

tlively Dec 19, 2023

aheejin Dec 20, 2023

tlively Dec 19, 2023

aheejin Dec 20, 2023

aheejin Dec 20, 2023

aheejin Dec 20, 2023 •

edited

Loading

tlively Dec 19, 2023

aheejin Dec 20, 2023

aheejin Dec 20, 2023

aheejin Dec 20, 2023

tlively left a comment

tlively Dec 20, 2023

aheejin Dec 20, 2023

		if (curr->type != Type::unreachable) {
		shouldBeSubType(curr->body->type,

	} else if (auto* tt = expr->dynCast<TryTable>()) {
	for (Index i = 0; i < tt->catchTags.size(); i++) {
	auto dest = tt->catchDests[i];
	if (dest == name) {
	func(name, tt->sentTypes[i]);
	}
	}

[EH] Add validation for new instructions #6185

[EH] Add validation for new instructions #6185

Conversation

aheejin commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aheejin Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlively left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aheejin Dec 20, 2023 •

edited

Loading