[interpreter] Add event section #151

thibaudmichaud · 2021-04-21T15:50:12Z

This change adds support for the event section in the interpreter. This is based on the explainer and on existing implementations (V8, wabt). The changes are mostly mechanical and follow the syntax and logic of function declarations, with the additional rule that the result type must be empty.

rossberg

Ah, this is great. It looks good overall, up to some details of the factorisation, see comments.

Sorry, I didn't know that you were up to this, otherwise I would have pointed you to the PR at effect-handlers/wasm-spec#4, where I already implemented event sections (though with some extensions for the continuation proposal). That PR also includes basic parts of the exception semantics, so feel free to steal form it to avoid duplicate work. It covers only a subset of the exception proposal's instruction set, though.

(One difference with your PR is that I did not use the indirection through I type index in the representation of event types. That was just laziness on my part, and is a TODO in the repo. Your encoding is more accurate.)

rossberg · 2021-04-22T10:55:23Z

interpreter/runtime/instance.ml

@@ -15,6 +16,7 @@ type module_inst =
 and func_inst = module_inst ref Func.t
 and table_inst = Table.t
 and memory_inst = Memory.t
+and event_inst = func_type


You'll need to introduce a new type for this, since events have identity. In my implementation, I introduced a new module Event defining, analogous to the other external entities.

rossberg · 2021-04-22T10:58:10Z

interpreter/syntax/types.ml

@@ -10,11 +10,13 @@ type 'a limits = {min : 'a; max : 'a option}
 type mutability = Immutable | Mutable
 type table_type = TableType of Int32.t limits * ref_type
 type memory_type = MemoryType of Int32.t limits
+type event_type = func_type


For symmetry with other types (and better extensibility):

type event_type = EventType of func_type

In fact, this ought to reflect the indirection through a type index in the binary format, so should be

Suggested change

type event_type = func_type

type event_type = EventType of int32

rossberg · 2021-04-22T11:00:08Z

interpreter/text/arrange.ml

+(* Events *)
+
+let event off i e =
+  Node ("event",


This could insert a symbolic name for better readability, analogous to the other forms of definition.

rossberg · 2021-04-22T11:03:44Z

interpreter/text/parser.mly

+  | LPAR RESULT value_type_list RPAR event_fields_import_result
+    { let FuncType (ins, out) = $5 in FuncType (ins, $3 @ out) }
+
+event_types :


Nit: event_type, since it represents a single event type. Also, I think you can define this as simply

event_type : | func_type { fun c -> EventType ($1 c) }

(assuming you added the EventType constructor as suggested above).

Good point, I simplified this even further by removing the new rule completely and using func_type directly.

rossberg · 2021-04-22T11:11:11Z

interpreter/binary/decode.ml

@@ -555,6 +556,7 @@ let import_desc s =
  | 0x01 -> TableImport (table_type s)
  | 0x02 -> MemoryImport (memory_type s)
  | 0x03 -> GlobalImport (global_type s)
+  | 0x04 -> ignore (vu32 s); EventImport (at var s)


You need to check that the value is 0, otherwise it won't be forward-compatible. Also, a single zero byte is enough for an unused extension hook (since it's upward-compatible to LEB), and is what we did elsewhere.

In similar other places, the decoder uses the zero function for this.

Moreover, I think you want to introduce an event_type decoding function for this, since the same it's reused for event definitions below. Something like:

let event_type s = zero s; let x = at var s in EventType x

rossberg · 2021-04-22T11:15:19Z

interpreter/binary/decode.ml

+(* Event section *)
+
+let event s =
+  ignore (vu32 s); var s


As mentioned above, this should probably invoke an event_type decoder function.

rossberg · 2021-04-22T11:17:39Z

interpreter/binary/encode.ml

@@ -426,6 +426,7 @@ let encode m =
      | TableImport t -> u8 0x01; table_type t
      | MemoryImport t -> u8 0x02; memory_type t
      | GlobalImport t -> u8 0x03; global_type t
+      | EventImport x -> u8 0x04; vu32 0x00l; var x


Similar to the decoder, I'd introduce an event_type encoding function.

thibaudmichaud · 2021-04-23T16:13:12Z

Thanks for the review! I was also not aware you were working on this, I will definitely take some inspiration.
I only plan to implement what is necessary to convert EH spec tests into JS tests for now, not the full runtime semantics yet. I can revisit the priority of that when EH is at phase 3 if no one else takes it.

rossberg

Sorry for the delay. This LGTM modulo a few nits.

There is an open question regarding the proper definition and use of event_type (see comment below), but I think we can resolve that later.

rossberg · 2021-05-05T06:30:26Z

interpreter/syntax/types.ml

@@ -10,11 +10,13 @@ type 'a limits = {min : 'a; max : 'a option}
 type mutability = Immutable | Mutable
 type table_type = TableType of Int32.t limits * ref_type
 type memory_type = MemoryType of Int32.t limits
+type event_type = EventType of int32


For consistency, can you change the other occurrences to int32 as well?

rossberg · 2021-05-05T06:37:02Z

interpreter/binary/encode.ml

@@ -118,6 +121,8 @@ let encode m =
    let memory_type = function
      | MemoryType lim -> limits vu32 lim

+    let event_type x = vu32 0x00l; var x


Suggested change

let event_type x = vu32 0x00l; var x

let event_type x = u8 0x00; var x

rossberg · 2021-05-05T06:38:31Z

interpreter/binary/encode.ml

@@ -426,6 +428,7 @@ let encode m =
      | TableImport t -> u8 0x01; table_type t
      | MemoryImport t -> u8 0x02; memory_type t
      | GlobalImport t -> u8 0x03; global_type t
+      | EventImport x -> u8 0x04; event_type x


Suggested change

| EventImport x -> u8 0x04; event_type x

| EventImport t -> u8 0x04; event_type t

rossberg · 2021-05-05T06:40:58Z

interpreter/runtime/event.ml

@@ -0,0 +1,10 @@
+open Types


Even if its kind of trivial, can you also add an .mli file for this module?

rossberg · 2021-05-05T06:46:08Z

interpreter/runtime/event.ml

@@ -0,0 +1,10 @@
+open Types
+
+type event = {ty : func_type}


Hm, it's a bit odd that this is func_type not event_type. I can see why, but it would break down once an event type has additional attributes.

The underlying problem is that there are two notions of event_type: the syntactic one (with a typeidx) and the semantic one (with the function type inline). Depending on context, we want one or the other. This is not a situation we've had before, so I don't have any great suggestion right now. We can resolve this later.

Ack. Regarding the syntactic vs semantic event_type: I don't see how this is too different from functions, where the type can also be represented directly or through the type ID? It makes sense to me that if one uses the "semantic" encoding, the other does too.

The difference is the extra indirection. For function types, the type section is just mapping a typeidx to a func type, and the functype itself is a closed type description. Once looked up, we can use that directly. But for event types, the definition itself refers to another typeidx, so isn't self-contained. This becomes clearer if we assume that an event type has additional ingredients, say, a hypothetical event_mode (as per the reserved zero byte). Then for the syntax, we'd need
event_type = EventType of typeidx * event_mode
But but when e.g. defining extern_type, that index is meaningless (because it's used outside any module) and you want ExternEventType of event_type' where
event_type' = EventType' of func_type * event_mode
FWIW, a similar problem also arises with typed reference types, so the syntactic vs semantic distinction becomes rather ubiquitous.

I agree, and for this reason I think types.ml should contain the resolved type.
We already have the typeidx in the AST:

type event = event' Source.phrase and event' = { etype : var; }

which makes sense for encoding, decoding and pretty-printing. For type checking, imports/exports and everything else, the resolved type is easier to work with. WDYT about simply reverting back to type event_type = EventType of func_type?

Actually, both etype and EventImport ought to carry an event_type, not a var. Otherwise, they in turn don't factorise correctly, e.g., when event types were to consist of more than just a var.

I'm afraid we can't hack around the two facets, they're inherent. It's relatively localised here, but becomes ubiquitous with concrete reference types. In the function references proposal I currently resolve it by actually distinguishing syntactic and semantic types. They differ by how variables are represented: for syntactic types it's just an index, with semantic types they become a pointer to the actual definition.

But I suggest to not worry about it for now. I want to play with streamlining this a bit and can fix it later. This needs a representation that cleanly carries over to the paper spec.

rossberg · 2021-05-05T06:49:34Z

interpreter/syntax/ast.ml

@@ -150,6 +150,8 @@ and memory' =
  mtype : memory_type;
 }

+type event = int32 Source.phrase


Suggested change

type event = int32 Source.phrase

type event = event' Source.phrase

and event' =

{

etype : var;

}

rossberg · 2021-05-05T06:53:58Z

test/core/exceptions.wast

+
+(module
+  (event)
+  (event (param i32))


Testing a few more syntactic variations (e.g. with symbolic names) couldn't harm.

Done. I did much more extensive testing with my next PR which includes the new EH opcodes. I'll send it for review after this one is merged.
In the meantime I added the relevant tests to this PR as suggested.

rossberg · 2021-05-05T06:54:56Z

test/core/exceptions.wast

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


Nit: Maybe name this file event.wast, since it's testing event definitions (exception-related instructions should go to separate files).

rossberg · 2021-05-05T14:30:59Z

interpreter/runtime/event.ml

@@ -0,0 +1,10 @@
+open Types
+
+type event = {ty : func_type}


The difference is the extra indirection. For function types, the type section is just mapping a typeidx to a func type, and the functype itself is a closed type description. Once looked up, we can use that directly. But for event types, the definition itself refers to another typeidx, so isn't self-contained. This becomes clearer if we assume that an event type has additional ingredients, say, a hypothetical event_mode (as per the reserved zero byte). Then for the syntax, we'd need
event_type = EventType of typeidx * event_mode
But but when e.g. defining extern_type, that index is meaningless (because it's used outside any module) and you want ExternEventType of event_type' where
event_type' = EventType' of func_type * event_mode
FWIW, a similar problem also arises with typed reference types, so the syntactic vs semantic distinction becomes rather ubiquitous.

[interpreter] Add event section

b3b3ed2

rossberg reviewed Apr 22, 2021

View reviewed changes

thibaudmichaud force-pushed the interpreter-events branch from f340dab to 8640005 Compare April 23, 2021 15:28

fixup! [interpreter] Add event section

3fc8553

thibaudmichaud force-pushed the interpreter-events branch from 8640005 to 3fc8553 Compare April 24, 2021 22:22

rossberg approved these changes May 5, 2021

View reviewed changes

fixup! [interpreter] Add event section

34925f9

thibaudmichaud force-pushed the interpreter-events branch from 75de259 to 34925f9 Compare May 5, 2021 13:34

rossberg approved these changes May 5, 2021

View reviewed changes

thibaudmichaud merged commit a5e87e4 into WebAssembly:master May 20, 2021

aheejin mentioned this pull request Jun 25, 2021

Initial support for exceptions in spec interpreter #28

Closed

aheejin mentioned this pull request Sep 7, 2021

Plan for Phase 4 #179

Open

11 tasks

	type event_type = func_type
	type event_type = EventType of int32

	let event_type x = vu32 0x00l; var x
	let event_type x = u8 0x00; var x

	\| EventImport x -> u8 0x04; event_type x
	\| EventImport t -> u8 0x04; event_type t

-type event = int32 Source.phrase
+type event = event' Source.phrase
+and event' =
+{
+  etype : var;
+}

[interpreter] Add event section #151

[interpreter] Add event section #151

Conversation

thibaudmichaud commented Apr 21, 2021

rossberg 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

thibaudmichaud Apr 23, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaudmichaud commented Apr 23, 2021

rossberg left a comment

Choose a reason for hiding this comment

rossberg May 5, 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

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

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 Apr 23, 2021 •

edited

Loading

rossberg May 5, 2021 •

edited

Loading