[EH] Make rethrow's target a try label by aheejin · Pull Request #3568 · WebAssembly/binaryen

aheejin · 2021-02-14T18:56:48Z

I was previously mistaken about rethrow's argument rule and thought
it only counted catch's depth. But it turns out it follows the same
rule delegate's label: the immediate argument follows the same rule as
when computing branch labels, but it only can target try labels
(semantically it targets that try's corresponding catch); otherwise
it will be a validation failure. Unlike delegate, rethrow's label
denotes not where to rethrow, but which exception to rethrow. For
example,

try $l0
catch ($l0)
  try $l1
  catch ($l1)
   rethrow $l0 ;; rethrow the exception caught by 'catch ($l0)'
  end
end

Refer to this comment for the more detailed informal semantics:
WebAssembly/exception-handling#146 (comment)

This also reverts some of delegateTarget -> exceptionTarget changes
done in #3562 in the validator. Label validation rules apply differently
for delegate and rethrow for try-catch. For example, this is valid:

try $l0
  try
  delegate $l0
catch ($l0)
end

But this is NOT valid:

try $l0
catch ($l0)
  try
  delegate $l0
end

So try's label should be used within try-catch range (not catch-end
range) for delegates.

But for the rethrow the rule is different. For example, this is valid:

try $l0
catch ($l0)
  rethrow $l0
end

But this is NOT valid:

try $l0
  rethrow $l0
catch ($l0)
end

So the try's label should be used within catch-end range instead.

I was previously mistaken about `rethrow`'s argument rule and thought it only counted `catch`'s depth. But it turns out it follows the same rule with `delegate`'s label: the immediate argument is comptuted in the same way as branch labels, but it only can target `try` labels (semantically it targets that `try`'s corresponding `catch`); otherwise it will be a validation failure. Unlike `delegate`, `rethrow`'s label denotes not where to rethrow, but which exception to rethrow. For example, ```wasm try $l0 catch ($l0) try $l1 catch ($l1) rethrow $l0 ;; rethrow the exception caught by 'catch ($l0)' end end ``` Refer to this comment for the more detailed informal semantics: WebAssembly/exception-handling#146 (comment) This also reverses some of `delegateTarget` -> `exceptionTarget` changes done in WebAssembly#3562 in the validator. Label validation rules apply differently for `delegate` and `rethrow` for try-catch. For example, this is valid: ```wasm try $l0 try delegate $l0 catch ($l0) end ``` But this is NOT valid: ```wasm try $l0 catch ($l0) try delegate $l0 end ``` So `try`'s label should be used within try-catch range (not catch-end range) for `delegate`s. But for the `rethrow` the rule is different. For example, this is valid: ```wasm try $l0 catch ($l0) rethrow $l0 end ``` But this is NOT valid: ```wasm try $l0 rethrow $l0 catch ($l0) end ``` So the `try`'s label should be used within catch-end range instead.

aheejin · 2021-02-14T19:05:34Z

src/wasm/wasm-s-parser.cpp

-Expression*
-SExpressionWasmBuilder::makeTryOrCatchBody(Element& s, Type type, bool isTry) {
-  if (isTry && !elementStartsWith(s, "do")) {
-    throw ParseException("invalid try do clause", s.line, s.col);
-  }
-  if (!isTry && !elementStartsWith(s, "catch") &&
-      !elementStartsWith(s, "catch_all")) {
-    throw ParseException("invalid catch clause", s.line, s.col);
-  }
-  if (s.size() == 1) { // (do) / (catch) / (catch_all) without instructions
-    return makeNop();
-  }
-  auto ret = allocator.alloc<Block>();
-  for (size_t i = 1; i < s.size(); i++) {
-    ret->list.push_back(parseExpression(s[i]));
-  }
-  if (ret->list.size() == 1) {
-    return ret->list[0];
-  }
-  ret->finalize(type);
-  return ret;
-}
-


This method was already unused; it became unused in #3487 but wasn't deleted there.

aheejin · 2021-02-14T19:06:02Z

test/passes/dce_all-features.wast

+          ;; This i32.add will be dce'd
+          (i32.add
+            (i32.const 0)
+            (rethrow $l0)


This function had to change because now rethrow should be within a catch.

src/wasm/wasm-binary.cpp

src/wasm/wasm-validator.cpp

tlively · 2021-02-16T02:04:07Z

src/wasm/wasm-validator.cpp

+  shouldBeTrue(delegateTargetNames.empty(),
               curr->body,
               "all named delegate targets must exist");
+  shouldBeTrue(rethrowTargetNames.empty(),
+               curr->body,
+               "all named rethrow targets must exist");


Couldn't these only fail if there was a bug in the validator itself? If so, should these be asserts instead?

Isn't that the case basically for all checking points within the validator? We already have similar lines:

binaryen/src/wasm/wasm-validator.cpp

Lines 2534 to 2538 in 1d3f578

shouldBeTrue(

breakInfos.empty(), curr->body, "all named break targets must exist");

shouldBeTrue(exceptionTargetNames.empty(),

curr->body,

"all named delegate targets must exist");

In other words, is it possible to construct an invalid Binaryen IR module that would trigger this error? (I would ask the same questions of any of the other checks we have)

Yeah I think so. Some flawed pass can assign a block label to delegate, or even a random string (unlikely), but anyway I don't think it's impossible.

But that would be caught by https://github.com/WebAssembly/binaryen/pull/3568/files#diff-2149be365b66a82038f1d3fa8f9fb1b4dcd40ab535c986f4bf0a4c37e669a1ceR2079-R2092, right? These checks are just checking that the target names were properly removed from their sets at the end of the corresponding scopes AFAICT, and it shouldn't be possible to create an invalid Binaryen module in which a try scope or catch scope never ends.

Hmm, I think you're right. Then maybe I'll remove all those checks..?

Oh no, you suggested to make them assertions... Will change them to assertions.

tlively

LGTM besides those few comments.

Co-authored-by: Thomas Lively <7121787+tlively@users.noreply.github.com>

aheejin requested review from kripken and tlively February 14, 2021 18:56

aheejin commented Feb 14, 2021

View reviewed changes

aheejin force-pushed the rethrow_fix branch from 8a56574 to 513c04f Compare February 14, 2021 20:01

tlively reviewed Feb 16, 2021

View reviewed changes

tlively approved these changes Feb 16, 2021

View reviewed changes

kripken approved these changes Feb 16, 2021

View reviewed changes

aheejin and others added 4 commits February 18, 2021 02:02

Update src/wasm/wasm-binary.cpp

6484eb3

Co-authored-by: Thomas Lively <7121787+tlively@users.noreply.github.com>

Address comments

21d808b

Remove redundant set empty checks at the end of validator

dd18b40

Change them to assertions

9cbff53

tlively approved these changes Feb 17, 2021

View reviewed changes

aheejin merged commit 1254b56 into WebAssembly:main Feb 17, 2021

aheejin deleted the rethrow_fix branch February 17, 2021 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EH] Make rethrow's target a try label#3568

[EH] Make rethrow's target a try label#3568
aheejin merged 5 commits intoWebAssembly:mainfrom
aheejin:rethrow_fix

aheejin commented Feb 14, 2021 •

edited

Loading

Uh oh!

aheejin Feb 14, 2021

Uh oh!

aheejin Feb 14, 2021

Uh oh!

Uh oh!

Uh oh!

tlively Feb 16, 2021

Uh oh!

aheejin Feb 17, 2021 •

edited

Loading

Uh oh!

tlively Feb 17, 2021

Uh oh!

aheejin Feb 17, 2021

Uh oh!

tlively Feb 17, 2021

Uh oh!

aheejin Feb 17, 2021

Uh oh!

aheejin Feb 17, 2021

Uh oh!

aheejin Feb 17, 2021

Uh oh!

tlively left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	shouldBeTrue(
	breakInfos.empty(), curr->body, "all named break targets must exist");
	shouldBeTrue(exceptionTargetNames.empty(),
	curr->body,
	"all named delegate targets must exist");

Conversation

aheejin commented Feb 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aheejin Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlively left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aheejin commented Feb 14, 2021 •

edited

Loading

aheejin Feb 17, 2021 •

edited

Loading