[interpreter] Parse and convert EH opcodes #160

thibaudmichaud · 2021-06-09T18:49:45Z

Please take a look. This adds support for EH opcodes in the parser, AST, encoder, decoder and formatter, and adds core spec tests. This follows the latest discussions that seem to have reached agreement: unwind is not supported (#156), and try blocks may have no handlers attached to them (#157).

This can already be used to convert the tests to JS, so this should be a useful starting point to ensure consistency across JS engines. The interpreter itself is still missing validation and execution.

rossberg · 2021-06-10T08:25:24Z

interpreter/syntax/ast.ml

+           handler                    (* handle exception *)
+  | Throw of var                      (* throw exception *)
+  | Rethrow of var                    (* rethrow exception *)
+and handler =


It's not worth introducing an auxiliary type here (and auxiliary functions for it in various places). Simply make it two separate instructions, TryCatch and TryDelegate. That they share an opcode/keyword is best considered a detail of the binary & textual representation.

interpreter/binary/decode.ml

rossberg · 2021-06-10T08:33:04Z

interpreter/binary/decode.ml

  | _ ->
    let pos = pos s in
    let e' = instr s in
    instr_block' s (Source.(e' @@ region s pos pos) :: es)

+and handler s =
+  let rec catch_list () =


Nit: avoid nesting functions unless they close over local variables in a non-trivial fashion.

rossberg · 2021-06-10T08:34:55Z

interpreter/syntax/free.ml


 and block (es : instr list) =
  let free = list instr es in {free with labels = shift free.labels}

+and handler = function
+  | Catch (cs, catch_all) ->
+    let rec catch_list = function


Same nit here.

rossberg · 2021-06-10T08:41:59Z

test/core/exceptions.wast

@@ -0,0 +1,429 @@
+;; Test exception handling support


Can you split this up into files focussing on individual instructions?

throw.wast
rethrow.wast
try_catch.wast
try_delegate.wast

takikawa · 2021-06-12T18:42:04Z

I was able to run these tests for SpiderMonkey as well using its test harness for spec tests. After some modifications to account for recent changes (e.g., bare try blocks) and test harness issues, I believe SM also agrees on all of these tests as well.

One issue I ran into, however, is that the core tests are using assert_trap to test uncaught exception cases. Would it make sense to instead extend the interpreter with a new assertion (assert_uncaught_exception <action>) for these cases? (since the spec defines uncaught exceptions as host-dependent behavior rather than a trap necessarily)

rossberg · 2021-06-15T11:53:36Z

interpreter/binary/decode.ml

+    let ca = if peek s = Some 0x19 then begin
+      ignore (u8 s);
+      Some (instr_block s)
+    end else
+      None


Suggested change

let ca = if peek s = Some 0x19 then begin

ignore (u8 s);

Some (instr_block s)

end else

None

let ca =

if peek s = Some 0x19 then begin

ignore (u8 s);

Some (instr_block s)

end else

None

rossberg · 2021-06-15T11:58:08Z

interpreter/binary/decode.ml

+    end else
+      begin match op s with
+      | 0x0b -> try_catch bt es [] None
+      | 0x18 -> try_delegate bt es (at var s)
+      | b -> illegal s pos b
+      end


thibaudmichaud · 2021-06-15T14:29:50Z

One issue I ran into, however, is that the core tests are using assert_trap to test uncaught exception cases. Would it make sense to instead extend the interpreter with a new assertion (assert_uncaught_exception ) for these cases? (since the spec defines uncaught exceptions as host-dependent behavior rather than a trap necessarily)

I think that makes sense and I just updated the PR with that suggestion. The JS implementation of the rule uses a simple try/catch to accept any exception.

I was actually planning to add such a rule later that uses the JS API to also check the type and content of the exception, but for now I think it is sufficient to add the weak form to solve the issue you pointed out.

takikawa · 2021-06-15T22:01:29Z

I think that makes sense and I just updated the PR with that suggestion. The JS implementation of the rule uses a simple try/catch to accept any exception.

Thanks! I just tried the new commit with assert_uncaught_exception and I was able to get it to work with the SpiderMonkey spec test harness.

rossberg · 2021-06-16T06:18:26Z

Ah, wanted to comment on the assertion question as well, but forgot.

I agree, we should have a dedicated assertion for that. But can we just call it assert_exception though? Assertions are kind of a unit test thing, so "uncaught" seems off, since that's only meaningful in the context of an entire program.

thibaudmichaud · 2021-06-16T08:18:27Z

I agree, we should have a dedicated assertion for that. But can we just call it assert_exception though? Assertions are kind of a unit test thing, so "uncaught" seems off, since that's only meaningful in the context of an entire program.

Done, both look fine to me.

aheejin

Thanks! I haven't read the ML implementation part yet but the test cases here match my understanding.

aheejin · 2021-06-17T04:29:38Z

test/core/rethrow.wast

+          (do (throw $e0))
+          (catch $e0
+            (if (i32.eq (local.get 0) (i32.const 0)) (then (rethrow 1)))
+	    (if (i32.eq (local.get 0) (i32.const 1)) (then (rethrow 2)))


Suggested change

(if (i32.eq (local.get 0) (i32.const 1)) (then (rethrow 2)))

(if (i32.eq (local.get 0) (i32.const 1)) (then (rethrow 2)))

aheejin · 2021-06-17T04:30:50Z

test/core/rethrow.wast

+	  (do (if (i32.eqz (local.get 0)) (then (rethrow 2))) (i32.const 42))
+	  (catch $e0 (i32.const 23))


Suggested change

(do (if (i32.eqz (local.get 0)) (then (rethrow 2))) (i32.const 42))

(catch $e0 (i32.const 23))

(do (if (i32.eqz (local.get 0)) (then (rethrow 2))) (i32.const 42))

(catch $e0 (i32.const 23))

Tab characters are mixed in there

aheejin · 2021-06-17T05:11:19Z

test/core/try_delegate.wast

+
+(assert_return (invoke "delegate-merge" (i32.const 1) (i32.const 0)) (i32.const 2))
+(assert_exception (invoke "delegate-merge" (i32.const 2) (i32.const 0)))
+(assert_return (invoke "delegate-merge" (i32.const 1)) (i32.const 2))


Is it OK to call a function having two parameters with just one parameter? Then are the remaining params are set to 0?

It should be ok but in this case that was a mistake, thanks for noticing! Fixed.

aheejin · 2021-06-17T06:13:18Z

test/core/try_catch.wast

+(module
+  (event $imported-e0 (import "test" "e0"))
+  (func $imported-throw (import "test" "throw"))
+  (event $e0)


Assuming that we are changing this to (tag ...) (not necessarily in this PR), do we need a text syntax to represent the attribute (exception)? Currently we are tentatively using

(event $e0 (attr 0))

in Binaryen, but not sure if this is a good one. (I uploaded a PR to change this in Binaryen to (tag ..) in WebAssembly/binaryen#3937, but this attr thing is a separate thing anyway)

As argued elsewhere, I don't think it is necessary to manifest the (so far vacuous) attribute in the text format for the time being. It's not even necessary to put it in the semantics. All we need right now is a reserved zero byte in the binary format, until we know better what possible distinctions are needed in the future.

So if we eventually need a different tag kind and go with @aheejin's syntax, exception tags would be a special case as shown below?

(tag) ;; an exception tag (tag (attr 0)) ;; an exception tag (tag (attr 1)) ;; some other tag

I am fine with that, just want to clarify.

Yes, it would default to the current meaning.

FWIW, it might not even be necessary that we distinguish tags by role. That is, it seems fine to use any tag as an exception, as long as it has a suitable type (with empty result). Provided there is no source of confusion between tags used in different roles.

Sounds good. I'll remove the attr attribute from the Binaryen text format then.

Add support for EH opcodes in the parser, AST, encoder, decoder and formatter, and add spec tests. This can already be used to convert the tests to JS, but not run them with the interpreter yet since validation and execution are still missing.

This attribute is always 0 and reserved for future use. In Binayren's unofficial text format we were writing this field as `(attr 0)`, but we have recently come to the conclusion that this is not necessary. Relevant discussion: WebAssembly/exception-handling#160 (comment)

ioannad

I agree with @aheejin that the change from event to tag could be done in a different PR, so everything LGTM. Sorry for the delayed review!

The assert_uncaught_exception assertion used for exception handling was renamed to assert_exception in the final version of WebAssembly/exception-handling#160 but was out-of-date in the wast crate.

thibaudmichaud requested review from takikawa, rossberg, aheejin and ioannad June 9, 2021 18:49

rossberg reviewed Jun 10, 2021

View reviewed changes

thibaudmichaud force-pushed the interpreter-eh branch from b0d52fb to 5e7081d Compare June 10, 2021 16:57

rossberg approved these changes Jun 15, 2021

View reviewed changes

thibaudmichaud force-pushed the interpreter-eh branch from 5e7081d to 6ad5c54 Compare June 15, 2021 14:17

takikawa approved these changes Jun 15, 2021

View reviewed changes

thibaudmichaud force-pushed the interpreter-eh branch from 6ad5c54 to 1f62ccf Compare June 16, 2021 07:56

aheejin reviewed Jun 17, 2021

View reviewed changes

[interpreter] Parse and convert EH opcodes

45b454d

Add support for EH opcodes in the parser, AST, encoder, decoder and formatter, and add spec tests. This can already be used to convert the tests to JS, but not run them with the interpreter yet since validation and execution are still missing.

thibaudmichaud force-pushed the interpreter-eh branch from 1f62ccf to 45b454d Compare June 17, 2021 11:17

aheejin mentioned this pull request Jun 18, 2021

Remove (attr 0) from tag text format WebAssembly/binaryen#3946

Merged

aheejin mentioned this pull request Jun 22, 2021

Advance to Phase 3? #163

Closed

ioannad approved these changes Jun 22, 2021

View reviewed changes

thibaudmichaud merged commit 7799129 into WebAssembly:master Jun 23, 2021

aheejin mentioned this pull request Jun 25, 2021

Initial support for exceptions in spec interpreter #28

Closed

This was referenced Jun 30, 2021

Implement event->tag renaming in core tests #169

Closed

Implement validation & evaluation in the spec interpreter #171

Closed

takikawa mentioned this pull request Aug 21, 2021

Rename assert_uncaught_exception to assert_exception bytecodealliance/wasm-tools#324

Merged

aheejin mentioned this pull request Sep 7, 2021

Plan for Phase 4 #179

Open

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[interpreter] Parse and convert EH opcodes #160

[interpreter] Parse and convert EH opcodes #160

thibaudmichaud commented Jun 9, 2021

rossberg Jun 10, 2021

thibaudmichaud Jun 10, 2021

rossberg Jun 10, 2021

thibaudmichaud Jun 10, 2021

rossberg Jun 10, 2021

thibaudmichaud Jun 10, 2021

rossberg Jun 10, 2021

thibaudmichaud Jun 10, 2021

takikawa commented Jun 12, 2021 •

edited

Loading

rossberg Jun 15, 2021

thibaudmichaud Jun 15, 2021

rossberg Jun 15, 2021

thibaudmichaud Jun 15, 2021

thibaudmichaud commented Jun 15, 2021

takikawa commented Jun 15, 2021 •

edited

Loading

rossberg commented Jun 16, 2021 •

edited

Loading

thibaudmichaud commented Jun 16, 2021

aheejin left a comment

aheejin Jun 17, 2021

thibaudmichaud Jun 17, 2021

aheejin Jun 17, 2021

thibaudmichaud Jun 17, 2021

aheejin Jun 17, 2021

thibaudmichaud Jun 17, 2021

aheejin Jun 17, 2021

rossberg Jun 17, 2021

thibaudmichaud Jun 17, 2021

rossberg Jun 17, 2021

aheejin Jun 17, 2021

ioannad left a comment

	(if (i32.eq (local.get 0) (i32.const 1)) (then (rethrow 2)))
	(if (i32.eq (local.get 0) (i32.const 1)) (then (rethrow 2)))

		(do (if (i32.eqz (local.get 0)) (then (rethrow 2))) (i32.const 42))
		(catch $e0 (i32.const 23))

[interpreter] Parse and convert EH opcodes #160

[interpreter] Parse and convert EH opcodes #160

Conversation

thibaudmichaud commented Jun 9, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takikawa commented Jun 12, 2021 • 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

thibaudmichaud commented Jun 15, 2021

takikawa commented Jun 15, 2021 • edited Loading

rossberg commented Jun 16, 2021 • edited Loading

thibaudmichaud commented Jun 16, 2021

aheejin left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioannad left a comment

Choose a reason for hiding this comment

takikawa commented Jun 12, 2021 •

edited

Loading

takikawa commented Jun 15, 2021 •

edited

Loading

rossberg commented Jun 16, 2021 •

edited

Loading