From 2e43c0cb2b41d3543148ab4f8444b81a6c98fdde Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Tue, 30 Jan 2024 06:24:39 -0800 Subject: [PATCH] [Parser] Parse pops (by doing nothing) (#6252) Parse pop expressions and check that they have the expected types, but do not actually create new Pop expressions or push anything onto the stack because we already create Pop expressions as necessary when visiting the beginning of catch blocks. Unlike the legacy text parser, the new text parser is not capable of parsing pops in invalid locations in the IR. This means that the new text parser will never be able to parse test/lit/catch-pop-fixup-eh-old.wast, which deliberately parses invalid IR to check that the pops can be fixed up and moved to the correct locations. It should be acceptable to delete that test when we turn on the new parser by default, though, so that won't be a problem. --- src/parser/contexts.h | 5 +++ src/parser/parsers.h | 4 +- src/wasm-ir-builder.h | 3 +- src/wasm/wasm-ir-builder.cpp | 24 +++++++++++- test/lit/wat-kitchen-sink.wast | 67 ++++++++++++++++++++++------------ 5 files changed, 77 insertions(+), 26 deletions(-) diff --git a/src/parser/contexts.h b/src/parser/contexts.h index 73be5e46ed0..c1ccefe53da 100644 --- a/src/parser/contexts.h +++ b/src/parser/contexts.h @@ -440,6 +440,7 @@ struct NullInstrParserCtx { Result<> makeMemoryCopy(Index, MemoryIdxT*, MemoryIdxT*) { return Ok{}; } Result<> makeMemoryFill(Index, MemoryIdxT*) { return Ok{}; } + template Result<> makePop(Index, TypeT) { return Ok{}; } Result<> makeCall(Index, FuncIdxT, bool) { return Ok{}; } template Result<> makeCallIndirect(Index, TableIdxT*, TypeUseT, bool) { @@ -1649,6 +1650,10 @@ struct ParseDefsCtx : TypeParserCtx { return withLoc(pos, irBuilder.makeMemoryFill(*m)); } + Result<> makePop(Index pos, Type type) { + return withLoc(pos, irBuilder.makePop(type)); + } + Result<> makeCall(Index pos, Name func, bool isReturn) { return withLoc(pos, irBuilder.makeCall(func, isReturn)); } diff --git a/src/parser/parsers.h b/src/parser/parsers.h index 706c6525ec4..c7b9168b11b 100644 --- a/src/parser/parsers.h +++ b/src/parser/parsers.h @@ -1555,7 +1555,9 @@ template Result<> makeMemoryFill(Ctx& ctx, Index pos) { } template Result<> makePop(Ctx& ctx, Index pos) { - return ctx.in.err("unimplemented instruction"); + auto type = valtype(ctx); + CHECK_ERR(type); + return ctx.makePop(pos, *type); } template Result<> makeCall(Ctx& ctx, Index pos, bool isReturn) { diff --git a/src/wasm-ir-builder.h b/src/wasm-ir-builder.h index fce8bd43120..d32dfebf205 100644 --- a/src/wasm-ir-builder.h +++ b/src/wasm-ir-builder.h @@ -145,7 +145,7 @@ class IRBuilder : public UnifiedExpressionVisitor> { [[nodiscard]] Result<> makeMemorySize(Name mem); [[nodiscard]] Result<> makeMemoryGrow(Name mem); [[nodiscard]] Result<> makeUnreachable(); - // [[nodiscard]] Result<> makePop(); + [[nodiscard]] Result<> makePop(Type type); [[nodiscard]] Result<> makeRefNull(HeapType type); [[nodiscard]] Result<> makeRefIsNull(); [[nodiscard]] Result<> makeRefFunc(Name func); @@ -232,6 +232,7 @@ class IRBuilder : public UnifiedExpressionVisitor> { [[nodiscard]] Result<> visitTupleExtract(TupleExtract*, std::optional arity = std::nullopt); + [[nodiscard]] Result<> visitPop(Pop*); private: Module& wasm; diff --git a/src/wasm/wasm-ir-builder.cpp b/src/wasm/wasm-ir-builder.cpp index 1eea777e01e..f8c8cf0a011 100644 --- a/src/wasm/wasm-ir-builder.cpp +++ b/src/wasm/wasm-ir-builder.cpp @@ -604,6 +604,12 @@ Result<> IRBuilder::visitTupleExtract(TupleExtract* curr, return Ok{}; } +Result<> IRBuilder::visitPop(Pop*) { + // Do not actually push this pop onto the stack since we generate our own pops + // as necessary when visiting the beginnings of try blocks. + return Ok{}; +} + Result<> IRBuilder::visitFunctionStart(Function* func) { if (!scopeStack.empty()) { return Err{"unexpected start of function"}; @@ -1300,7 +1306,23 @@ Result<> IRBuilder::makeUnreachable() { return Ok{}; } -// Result<> IRBuilder::makePop() {} +Result<> IRBuilder::makePop(Type type) { + // We don't actually want to create a new Pop expression here because we + // already create them automatically when starting a legacy catch block that + // needs one. Just verify that the Pop we are being asked to make is the same + // type as the Pop we have already made. + auto& scope = getScope(); + if (!scope.getCatch() || scope.exprStack.size() != 1 || + !scope.exprStack[0]->is()) { + return Err{ + "pop instructions may only appear at the beginning of catch blocks"}; + } + auto expectedType = scope.exprStack[0]->type; + if (type != expectedType) { + return Err{std::string("Expected pop of type ") + expectedType.toString()}; + } + return Ok{}; +} Result<> IRBuilder::makeRefNull(HeapType type) { push(builder.makeRefNull(type)); diff --git a/test/lit/wat-kitchen-sink.wast b/test/lit/wat-kitchen-sink.wast index 638d8000437..68046893533 100644 --- a/test/lit/wat-kitchen-sink.wast +++ b/test/lit/wat-kitchen-sink.wast @@ -13,11 +13,11 @@ (type $ret2 (func (result i32 i32))) (rec - ;; CHECK: (type $pair (struct (field (mut i32)) (field (mut i64)))) + ;; CHECK: (type $3 (func (result i32 i64))) - ;; CHECK: (type $4 (func (param i32 i64))) + ;; CHECK: (type $pair (struct (field (mut i32)) (field (mut i64)))) - ;; CHECK: (type $5 (func (result i32 i64))) + ;; CHECK: (type $5 (func (param i32 i64))) ;; CHECK: (type $a2 (array (mut f32))) @@ -1815,8 +1815,8 @@ end ) - ;; CHECK: (func $try-catch-params (type $5) (result i32 i64) - ;; CHECK-NEXT: (try (type $5) (result i32 i64) + ;; CHECK: (func $try-catch-params (type $3) (result i32 i64) + ;; CHECK-NEXT: (try (type $3) (result i32 i64) ;; CHECK-NEXT: (do ;; CHECK-NEXT: (tuple.make 2 ;; CHECK-NEXT: (i32.const 0) @@ -1836,6 +1836,27 @@ end ) + ;; CHECK: (func $try-catch-pop (type $3) (result i32 i64) + ;; CHECK-NEXT: (try (type $3) (result i32 i64) + ;; CHECK-NEXT: (do + ;; CHECK-NEXT: (tuple.make 2 + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: (i64.const 1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (catch $tag-pair + ;; CHECK-NEXT: (pop (tuple i32 i64)) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $try-catch-pop (result i32 i64) + try (result i32 i64) + i32.const 0 + i64.const 1 + catch $tag-pair + pop (tuple i32 i64) + end + ) ;; CHECK: (func $try-catch_all (type $void) ;; CHECK-NEXT: (try @@ -2674,8 +2695,8 @@ br 0 ) - ;; CHECK: (func $br-multivalue (type $5) (result i32 i64) - ;; CHECK-NEXT: (block $label (type $5) (result i32 i64) + ;; CHECK: (func $br-multivalue (type $3) (result i32 i64) + ;; CHECK-NEXT: (block $label (type $3) (result i32 i64) ;; CHECK-NEXT: (br $label ;; CHECK-NEXT: (tuple.make 2 ;; CHECK-NEXT: (i32.const 0) @@ -2690,9 +2711,9 @@ br 0 ) - ;; CHECK: (func $br-multivalue-drop (type $5) (result i32 i64) - ;; CHECK-NEXT: (block $label (type $5) (result i32 i64) - ;; CHECK-NEXT: (block (type $5) (result i32 i64) + ;; CHECK: (func $br-multivalue-drop (type $3) (result i32 i64) + ;; CHECK-NEXT: (block $label (type $3) (result i32 i64) + ;; CHECK-NEXT: (block (type $3) (result i32 i64) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (f32.const 0) ;; CHECK-NEXT: ) @@ -2892,9 +2913,9 @@ end ) - ;; CHECK: (func $br-table-multivalue (type $5) (result i32 i64) - ;; CHECK-NEXT: (block $a (type $5) (result i32 i64) - ;; CHECK-NEXT: (block $b (type $5) (result i32 i64) + ;; CHECK: (func $br-table-multivalue (type $3) (result i32 i64) + ;; CHECK-NEXT: (block $a (type $3) (result i32 i64) + ;; CHECK-NEXT: (block $b (type $3) (result i32 i64) ;; CHECK-NEXT: (br_table $a $b ;; CHECK-NEXT: (tuple.make 2 ;; CHECK-NEXT: (i32.const 42) @@ -3049,7 +3070,7 @@ drop ) - ;; CHECK: (func $memory-grow (type $4) (param $0 i32) (param $1 i64) + ;; CHECK: (func $memory-grow (type $5) (param $0 i32) (param $1 i64) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (memory.grow $mimport$0 ;; CHECK-NEXT: (local.get $0) @@ -3107,7 +3128,7 @@ global.set $pair ) - ;; CHECK: (func $load (type $4) (param $0 i32) (param $1 i64) + ;; CHECK: (func $load (type $5) (param $0 i32) (param $1 i64) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (i32.load $mimport$0 offset=42 ;; CHECK-NEXT: (local.get $0) @@ -3136,7 +3157,7 @@ drop ) - ;; CHECK: (func $store (type $4) (param $0 i32) (param $1 i64) + ;; CHECK: (func $store (type $5) (param $0 i32) (param $1 i64) ;; CHECK-NEXT: (i32.store $mimport$0 offset=42 align=1 ;; CHECK-NEXT: (local.get $0) ;; CHECK-NEXT: (i32.const 0) @@ -3162,7 +3183,7 @@ f32.store $mem-i64 ) - ;; CHECK: (func $atomic-rmw (type $4) (param $0 i32) (param $1 i64) + ;; CHECK: (func $atomic-rmw (type $5) (param $0 i32) (param $1 i64) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (i32.atomic.rmw16.add_u $mimport$0 ;; CHECK-NEXT: (local.get $0) @@ -3187,7 +3208,7 @@ drop ) - ;; CHECK: (func $atomic-cmpxchg (type $4) (param $0 i32) (param $1 i64) + ;; CHECK: (func $atomic-cmpxchg (type $5) (param $0 i32) (param $1 i64) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (i32.atomic.rmw8.cmpxchg_u $mem ;; CHECK-NEXT: (local.get $0) @@ -3216,7 +3237,7 @@ drop ) - ;; CHECK: (func $atomic-wait (type $4) (param $0 i32) (param $1 i64) + ;; CHECK: (func $atomic-wait (type $5) (param $0 i32) (param $1 i64) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (memory.atomic.wait32 $mimport$0 ;; CHECK-NEXT: (local.get $0) @@ -3245,7 +3266,7 @@ drop ) - ;; CHECK: (func $atomic-notify (type $4) (param $0 i32) (param $1 i64) + ;; CHECK: (func $atomic-notify (type $5) (param $0 i32) (param $1 i64) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (memory.atomic.notify $mimport$0 offset=8 ;; CHECK-NEXT: (local.get $0) @@ -3337,7 +3358,7 @@ i8x16.shl ) - ;; CHECK: (func $simd-load (type $4) (param $0 i32) (param $1 i64) + ;; CHECK: (func $simd-load (type $5) (param $0 i32) (param $1 i64) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (v128.load8x8_s $mimport$0 offset=8 ;; CHECK-NEXT: (local.get $0) @@ -3453,7 +3474,7 @@ memory.copy $mem-i64 5 ) - ;; CHECK: (func $memory-fill (type $4) (param $0 i32) (param $1 i64) + ;; CHECK: (func $memory-fill (type $5) (param $0 i32) (param $1 i64) ;; CHECK-NEXT: (memory.fill $mimport$0 ;; CHECK-NEXT: (local.get $0) ;; CHECK-NEXT: (i32.const 1) @@ -3568,7 +3589,7 @@ (func $ref-func ref.func $ref-func drop - ref.func 153 + ref.func 154 drop )