From a6085025d3ae24e438e9c9ff71061c23646cb5ae Mon Sep 17 00:00:00 2001 From: Asumu Takikawa Date: Thu, 29 Oct 2020 18:03:58 -0700 Subject: [PATCH] Add support for new exception handling proposal The exception handling proposal switched its design from an exnref-based approach with first-class exception reference values to a version with tagged catch blocks and implicit exception references. This commit adds support for this new semantics and revises tests to match. It does not yet remove exnref, as that can be done in a separate patch. Note: it does not add the `delegate` instruction in the new proposal yet, as it does not yet have an opcode assigned. --- crates/wasmparser/src/binary_reader.rs | 9 +-- crates/wasmparser/src/operators_validator.rs | 63 ++++++++++++++------ crates/wasmparser/src/primitives.rs | 6 +- crates/wasmprinter/src/lib.rs | 19 +++--- crates/wast/src/ast/expr.rs | 58 ++++++++++++++---- crates/wast/src/ast/mod.rs | 2 + crates/wast/src/resolve/names.rs | 9 ++- tests/local/exception-handling.wast | 58 +++++++++++++----- tests/local/try.wast | 15 +++-- tests/local/try.wat | 11 +++- tests/roundtrip.rs | 18 ++++++ 11 files changed, 194 insertions(+), 74 deletions(-) diff --git a/crates/wasmparser/src/binary_reader.rs b/crates/wasmparser/src/binary_reader.rs index 1f2b3c97e..147705d2c 100644 --- a/crates/wasmparser/src/binary_reader.rs +++ b/crates/wasmparser/src/binary_reader.rs @@ -1101,15 +1101,16 @@ impl<'a> BinaryReader<'a> { 0x06 => Operator::Try { ty: self.read_blocktype()?, }, - 0x07 => Operator::Catch, + 0x07 => Operator::Catch { + index: self.read_var_u32()?, + }, 0x08 => Operator::Throw { index: self.read_var_u32()?, }, - 0x09 => Operator::Rethrow, - 0x0a => Operator::BrOnExn { + 0x09 => Operator::Rethrow { relative_depth: self.read_var_u32()?, - index: self.read_var_u32()?, }, + 0x0a => Operator::Unwind, 0x0b => Operator::End, 0x0c => Operator::Br { relative_depth: self.read_var_u32()?, diff --git a/crates/wasmparser/src/operators_validator.rs b/crates/wasmparser/src/operators_validator.rs index 991f337b1..24bdfbda4 100644 --- a/crates/wasmparser/src/operators_validator.rs +++ b/crates/wasmparser/src/operators_validator.rs @@ -112,6 +112,8 @@ enum FrameKind { Loop, Try, Catch, + CatchAll, + Unwind, } impl OperatorValidator { @@ -567,10 +569,23 @@ impl OperatorValidator { } Operator::Else => { let frame = self.pop_ctrl(resources)?; - if frame.kind != FrameKind::If { - bail_op_err!("else found outside of an `if` block"); + // The `catch_all` instruction shares an opcode with `else`, + // so we check the frame to see how it's interpreted. + if self.features.exceptions + && (frame.kind == FrameKind::Try || frame.kind == FrameKind::Catch) + { + self.control.push(Frame { + kind: FrameKind::CatchAll, + block_type: frame.block_type, + height: self.operands.len(), + unreachable: false, + }); + } else { + if frame.kind != FrameKind::If { + bail_op_err!("else found outside of an `if` block"); + } + self.push_ctrl(FrameKind::Else, frame.block_type, resources)?; } - self.push_ctrl(FrameKind::Else, frame.block_type, resources)?; } Operator::Try { ty } => { self.check_exceptions_enabled()?; @@ -580,10 +595,10 @@ impl OperatorValidator { } self.push_ctrl(FrameKind::Try, ty, resources)?; } - Operator::Catch => { + Operator::Catch { index } => { self.check_exceptions_enabled()?; let frame = self.pop_ctrl(resources)?; - if frame.kind != FrameKind::Try { + if frame.kind != FrameKind::Try && frame.kind != FrameKind::Catch { bail_op_err!("catch found outside of an `try` block"); } // Start a new frame and push `exnref` value. @@ -593,7 +608,11 @@ impl OperatorValidator { height: self.operands.len(), unreachable: false, }); - self.push_operand(Type::ExnRef)?; + // Push exception argument types. + let ty = func_type_at(&resources, index)?; + for ty in ty.inputs() { + self.push_operand(ty)?; + } } Operator::Throw { index } => { self.check_exceptions_enabled()?; @@ -607,24 +626,30 @@ impl OperatorValidator { } self.unreachable(); } - Operator::Rethrow => { + Operator::Rethrow { relative_depth } => { self.check_exceptions_enabled()?; - self.pop_operand(Some(Type::ExnRef))?; + // This is not a jump, but we need to check that the `rethrow` + // targets an actual `catch` to get the exception. + let (_, kind) = self.jump(relative_depth)?; + if kind != FrameKind::Catch && kind != FrameKind::CatchAll { + bail_op_err!("rethrow target was not a `catch` block"); + } self.unreachable(); } - Operator::BrOnExn { - relative_depth, - index, - } => { + Operator::Unwind => { self.check_exceptions_enabled()?; - let (ty, kind) = self.jump(relative_depth)?; - self.pop_operand(Some(Type::ExnRef))?; - // Check the exception's argument values with target block's. - let exn_args = func_type_at(&resources, index)?; - if Iterator::ne(exn_args.inputs(), label_types(ty, resources, kind)?) { - bail_op_err!("target block types do not match"); + // Switch from `try` to an `unwind` frame, so we can check that + // the result type is empty. + let frame = self.pop_ctrl(resources)?; + if frame.kind != FrameKind::Try { + bail_op_err!("unwind found outside of an `try` block"); } - self.push_operand(Type::ExnRef)?; + self.control.push(Frame { + kind: FrameKind::Unwind, + block_type: TypeOrFuncType::Type(Type::EmptyBlockType), + height: self.operands.len(), + unreachable: false, + }); } Operator::End => { let mut frame = self.pop_ctrl(resources)?; diff --git a/crates/wasmparser/src/primitives.rs b/crates/wasmparser/src/primitives.rs index a8fedb796..3f92a09ed 100644 --- a/crates/wasmparser/src/primitives.rs +++ b/crates/wasmparser/src/primitives.rs @@ -347,10 +347,10 @@ pub enum Operator<'a> { If { ty: TypeOrFuncType }, Else, Try { ty: TypeOrFuncType }, - Catch, + Catch { index: u32 }, Throw { index: u32 }, - Rethrow, - BrOnExn { relative_depth: u32, index: u32 }, + Rethrow { relative_depth: u32 }, + Unwind, End, Br { relative_depth: u32 }, BrIf { relative_depth: u32 }, diff --git a/crates/wasmprinter/src/lib.rs b/crates/wasmprinter/src/lib.rs index 066b5fd5c..9ed318548 100644 --- a/crates/wasmprinter/src/lib.rs +++ b/crates/wasmprinter/src/lib.rs @@ -665,7 +665,7 @@ impl Printer { // `else`/`catch` are special in that it's printed at // the previous indentation, but it doesn't actually change // our nesting level. - Operator::Else | Operator::Catch => { + Operator::Else | Operator::Catch { .. } | Operator::Unwind => { self.nesting -= 1; self.newline(); self.nesting += 1; @@ -729,24 +729,21 @@ impl Printer { self.print_blockty(ty)?; write!(self.result, " ;; label = @{}", cur_label)?; } - Catch => self.result.push_str("catch"), + Catch { index } => { + write!(self.result, "catch {}", index)?; + } Throw { index } => { write!(self.result, "throw {}", index)?; } - Rethrow => self.result.push_str("rethrow"), - BrOnExn { - relative_depth, - index, - } => { + Rethrow { relative_depth } => { write!( self.result, - "br_on_exn {} (;{};)", + "rethrow {} (;{};)", relative_depth, - label(*relative_depth), + label(*relative_depth) )?; - write!(self.result, " {}", index)?; } - + Unwind => self.result.push_str("unwind"), End => self.result.push_str("end"), Br { relative_depth } => { write!( diff --git a/crates/wast/src/ast/expr.rs b/crates/wast/src/ast/expr.rs index b771c9609..1dd3bd0eb 100644 --- a/crates/wast/src/ast/expr.rs +++ b/crates/wast/src/ast/expr.rs @@ -84,7 +84,9 @@ enum If<'a> { enum Try<'a> { /// Next thing to parse is the `do` block. Do(Instruction<'a>), - /// Next thing to parse is the `catch` block. + /// Next thing to parse is `catch`/`catch_all`, or `unwind`. + CatchOrUnwind, + /// Next thing to parse is a `catch` block or `catch_all`. Catch, /// This `try` statement has finished parsing and if anything remains it's a /// syntax error. @@ -195,8 +197,8 @@ impl<'a> ExpressionParser<'a> { Level::Try(Try::Do(_)) => { return Err(parser.error("previous `try` had no `do`")); } - Level::Try(Try::Catch) => { - return Err(parser.error("previous `try` had no `catch`")); + Level::Try(Try::CatchOrUnwind) => { + return Err(parser.error("previous `try` had no `catch`, `catch_all`, or `unwind`")); } Level::Try(_) => { self.instrs.push(Instruction::End(None)); @@ -305,7 +307,7 @@ impl<'a> ExpressionParser<'a> { /// than an `if` as the syntactic form is: /// /// ```wat - /// (try (do $do) (catch $catch)) + /// (try (do $do) (catch $event $catch)) /// ``` /// /// where the `do` and `catch` keywords are mandatory, even for an empty @@ -328,7 +330,7 @@ impl<'a> ExpressionParser<'a> { if parser.parse::>()?.is_some() { // The state is advanced here only if the parse succeeds in // order to strictly require the keyword. - *i = Try::Catch; + *i = Try::CatchOrUnwind; self.stack.push(Level::TryArm); return Ok(true); } @@ -338,10 +340,26 @@ impl<'a> ExpressionParser<'a> { return Ok(false); } - // `catch` handled similar to `do`, including requiring the keyword. - if let Try::Catch = i { - self.instrs.push(Instruction::Catch); + // After a try's `do`, there are several possible kinds of handlers. + if let Try::CatchOrUnwind = i { + // `catch` may be followed by more `catch`s or `catch_all`. if parser.parse::>()?.is_some() { + let evt = parser.parse::>()?; + self.instrs.push(Instruction::Catch(evt)); + *i = Try::Catch; + self.stack.push(Level::TryArm); + return Ok(true); + } + // `catch_all` can only come at the end and has no argument. + if parser.parse::>()?.is_some() { + self.instrs.push(Instruction::CatchAll); + *i = Try::End; + self.stack.push(Level::TryArm); + return Ok(true); + } + // `unwind` is similar to `catch_all`. + if parser.parse::>()?.is_some() { + self.instrs.push(Instruction::Unwind); *i = Try::End; self.stack.push(Level::TryArm); return Ok(true); @@ -349,6 +367,23 @@ impl<'a> ExpressionParser<'a> { return Ok(false); } + if let Try::Catch = i { + if parser.parse::>()?.is_some() { + let evt = parser.parse::>()?; + self.instrs.push(Instruction::Catch(evt)); + *i = Try::Catch; + self.stack.push(Level::TryArm); + return Ok(true); + } + if parser.parse::>()?.is_some() { + self.instrs.push(Instruction::CatchAll); + *i = Try::End; + self.stack.push(Level::TryArm); + return Ok(true); + } + return Err(parser.error("unexpected items after `catch`")); + } + Err(parser.error("too many payloads inside of `(try)`")) } } @@ -997,11 +1032,12 @@ instructions! { V128Load64Zero(MemArg<8>) : [0xfd, 0xfd] : "v128.load64_zero", // Exception handling proposal + CatchAll : [0x05] : "catch_all", // Reuses the else opcode. Try(BlockType<'a>) : [0x06] : "try", - Catch : [0x07] : "catch", + Catch(ast::Index<'a>) : [0x07] : "catch", Throw(ast::Index<'a>) : [0x08] : "throw", - Rethrow : [0x09] : "rethrow", - BrOnExn(BrOnExn<'a>) : [0x0a] : "br_on_exn", + Rethrow(ast::Index<'a>) : [0x09] : "rethrow", + Unwind : [0x0a] : "unwind", } } diff --git a/crates/wast/src/ast/mod.rs b/crates/wast/src/ast/mod.rs index da5ebd9b2..aeb555896 100644 --- a/crates/wast/src/ast/mod.rs +++ b/crates/wast/src/ast/mod.rs @@ -346,6 +346,7 @@ pub mod kw { custom_keyword!(binary); custom_keyword!(block); custom_keyword!(catch); + custom_keyword!(catch_all); custom_keyword!(code); custom_keyword!(data); custom_keyword!(declare); @@ -416,6 +417,7 @@ pub mod kw { custom_keyword!(table); custom_keyword!(then); custom_keyword!(r#try = "try"); + custom_keyword!(unwind); custom_keyword!(v128); } diff --git a/crates/wast/src/resolve/names.rs b/crates/wast/src/resolve/names.rs index d7f6e3ee0..8062cee11 100644 --- a/crates/wast/src/resolve/names.rs +++ b/crates/wast/src/resolve/names.rs @@ -1950,10 +1950,13 @@ impl<'a, 'b> ExprResolver<'a, 'b> { Throw(i) => { self.module.resolve(i, Ns::Event)?; } - BrOnExn(b) => { - self.resolve_label(&mut b.label)?; - self.module.resolve(&mut b.exn, Ns::Event)?; + Rethrow(i) => { + self.resolve_label(i)?; } + Catch(i) => { + self.module.resolve(i, Ns::Event)?; + } + BrOnCast(b) => { self.resolve_label(&mut b.label)?; self.module.resolve_heaptype(&mut b.val)?; diff --git a/tests/local/exception-handling.wast b/tests/local/exception-handling.wast index ba97155ca..6f19e4eeb 100644 --- a/tests/local/exception-handling.wast +++ b/tests/local/exception-handling.wast @@ -1,29 +1,57 @@ ;; --enable-exceptions --enable-multi-value (module (type (func (param i32 i64))) + (type (func (param i32))) (event (type 0)) - (func $check-br_on_exn-rethrow (param exnref) - block $l (result i32 i64) - local.get 0 - ;; exnref $e is on the stack at this point - br_on_exn $l 0 ;; branch to $l with $e's arguments - rethrow - end - drop - drop - ) + (event (type 1)) (func $check-throw i32.const 1 i64.const 2 throw 0 ) - (func $check-try-w-calls (result i32) - try (result i32) + (func $check-try-catch-rethrow + try (result i32 i64) call $check-throw - i32.const 0 - catch - call $check-br_on_exn-rethrow + unreachable + catch 0 + ;; the exception arguments are on the stack at this point + catch 1 + i64.const 2 + catch_all + rethrow 0 + end + drop + drop + ) + (func $check-unwind (local i32) + try i32.const 1 + local.set 0 + call $check-throw + unwind + i32.const 0 + local.set 0 end ) ) + +(assert_invalid + (module + (func try catch_all catch_all end)) + ;; we can't distinguish between `catch_all` and `else` in error cases + "else found outside of an `if` block") + +(assert_invalid + (module + (func try catch_all catch 0 end)) + "catch found outside of an `try` block") + +(assert_invalid + (module + (func try unwind i32.const 1 end)) + "type mismatch: values remaining on stack at end of block") + +(assert_invalid + (module + (func block try catch_all rethrow 1 end end)) + "rethrow target was not a `catch` block") diff --git a/tests/local/try.wast b/tests/local/try.wast index 555af254b..51256ed07 100644 --- a/tests/local/try.wast +++ b/tests/local/try.wast @@ -2,13 +2,13 @@ (assert_malformed (module quote - "(func (try (catch)))" + "(func (try (catch $exn)))" ) "previous `try` had no `do`") (assert_malformed (module quote - "(func (try (unreachable) (catch)))" + "(func (try (unreachable) (catch $exn)))" ) "previous `try` had no `do`") @@ -26,13 +26,18 @@ (assert_malformed (module quote - "(func (try (do) (catch) drop))" + "(func (try (do) (catch $exn) drop))" ) "expected `(`") (assert_malformed (module quote - "(func (try (do) (catch) (drop)))" + "(func (try (do) (catch $exn) (drop)))" ) - "too many payloads inside of `(try)`") + "unexpected items after `catch`") +(assert_malformed + (module quote + "(func (try (do) (unwind) (drop)))" + ) + "too many payloads inside of `(try)`") diff --git a/tests/local/try.wat b/tests/local/try.wat index ae7b209cb..40c711a46 100644 --- a/tests/local/try.wat +++ b/tests/local/try.wat @@ -1,9 +1,14 @@ ;; --enable-exceptions (module $m - (func (try (do) (catch drop))) - (func (try (do) (catch rethrow))) + (type (func)) + (event $exn (type 0)) + (func (try (do) (catch $exn drop))) + (func (try (do) (catch $exn rethrow 0))) + (func (try (do) (catch_all rethrow 0))) + (func (try (do) (catch $exn) (catch_all rethrow 0))) + (func (try (do) (unwind nop))) (func (result i32) (try (result i32) (do (i32.const 42)) - (catch drop (i32.const 42))))) + (catch $exn drop (i32.const 42))))) diff --git a/tests/roundtrip.rs b/tests/roundtrip.rs index 14ba6fbf1..e8c51dc75 100644 --- a/tests/roundtrip.rs +++ b/tests/roundtrip.rs @@ -135,6 +135,21 @@ fn skip_test(test: &Path, contents: &[u8]) -> bool { "interp/reference-types.txt", "expr/reference-types.txt", "parse/all-features.txt", + // uses old exception handling proposal + // FIXME: remove when wabt starts supporting the new proposal + "desugar/try.txt", + "dump/br_on_exn.txt", + "dump/rethrow.txt", + "dump/try.txt", + "dump/try-multi.txt", + "expr/br_on_exn.txt", + "expr/rethrow.txt", + "expr/try.txt", + "expr/try-multi.txt", + "roundtrip/fold-br_on_exn.txt", + "roundtrip/fold-rethrow.txt", + "roundtrip/fold-try.txt", + "roundtrip/rethrow.txt", ]; if broken.iter().any(|x| test.ends_with(x)) { return true; @@ -253,6 +268,9 @@ impl TestState { && !test.ends_with("reference-types/binary.wast") && !test.ends_with("exception-handling/binary.wast") + // wabt uses the old exceptions proposal + && !test.ends_with("local/exception-handling.wast") + // not implemented in wabt && !test.iter().any(|t| t == "module-linking") && !test.ends_with("multi-memory.wast")