From 6864626c83ac5b1d28848698dbe985b3e9134ea6 Mon Sep 17 00:00:00 2001 From: SoniEx2 Date: Sat, 5 Oct 2024 09:37:51 -0300 Subject: [PATCH 1/3] Fix interp exception handling correctly --- include/wabt/binary-reader-logging.h | 1 + include/wabt/binary-reader-nop.h | 1 + include/wabt/binary-reader.h | 1 + src/binary-reader-logging.cc | 1 + src/binary-reader.cc | 1 + src/interp/binary-reader-interp.cc | 32 +++++++++++++++++----------- test/interp/basic-logging.txt | 1 + 7 files changed, 25 insertions(+), 13 deletions(-) diff --git a/include/wabt/binary-reader-logging.h b/include/wabt/binary-reader-logging.h index 4bb16deb28..fee211f96e 100644 --- a/include/wabt/binary-reader-logging.h +++ b/include/wabt/binary-reader-logging.h @@ -130,6 +130,7 @@ class BinaryReaderLogging : public BinaryReaderDelegate { Result BeginFunctionBody(Index index, Offset size) override; Result OnLocalDeclCount(Index count) override; Result OnLocalDecl(Index decl_index, Index count, Type type) override; + Result EndLocalDecls() override; Result OnOpcode(Opcode opcode) override; Result OnOpcodeBare() override; diff --git a/include/wabt/binary-reader-nop.h b/include/wabt/binary-reader-nop.h index ffa3a4b75a..c7ec78b11d 100644 --- a/include/wabt/binary-reader-nop.h +++ b/include/wabt/binary-reader-nop.h @@ -172,6 +172,7 @@ class BinaryReaderNop : public BinaryReaderDelegate { Result OnLocalDecl(Index decl_index, Index count, Type type) override { return Result::Ok; } + Result EndLocalDecls() override { return Result::Ok; } /* Function expressions; called between BeginFunctionBody and EndFunctionBody */ diff --git a/include/wabt/binary-reader.h b/include/wabt/binary-reader.h index de7c4e8fd8..3d48f57470 100644 --- a/include/wabt/binary-reader.h +++ b/include/wabt/binary-reader.h @@ -188,6 +188,7 @@ class BinaryReaderDelegate { virtual Result BeginFunctionBody(Index index, Offset size) = 0; virtual Result OnLocalDeclCount(Index count) = 0; virtual Result OnLocalDecl(Index decl_index, Index count, Type type) = 0; + virtual Result EndLocalDecls() = 0; /* Function expressions; called between BeginFunctionBody and EndFunctionBody */ diff --git a/src/binary-reader-logging.cc b/src/binary-reader-logging.cc index 653e1e487a..ea42739190 100644 --- a/src/binary-reader-logging.cc +++ b/src/binary-reader-logging.cc @@ -795,6 +795,7 @@ DEFINE_BEGIN(BeginCodeSection) DEFINE_INDEX(OnFunctionBodyCount) DEFINE_INDEX(EndFunctionBody) DEFINE_INDEX(OnLocalDeclCount) +DEFINE0(EndLocalDecls) DEFINE_LOAD_STORE_OPCODE(OnAtomicLoadExpr); DEFINE_LOAD_STORE_OPCODE(OnAtomicRmwExpr); DEFINE_LOAD_STORE_OPCODE(OnAtomicRmwCmpxchgExpr); diff --git a/src/binary-reader.cc b/src/binary-reader.cc index b4ad5b0c40..b68562dd50 100644 --- a/src/binary-reader.cc +++ b/src/binary-reader.cc @@ -2815,6 +2815,7 @@ Result BinaryReader::ReadCodeSection(Offset section_size) { ERROR_UNLESS(IsConcreteType(local_type), "expected valid local type"); CALLBACK(OnLocalDecl, k, num_local_types, local_type); } + CALLBACK(EndLocalDecls); if (options_.skip_function_bodies) { state_.offset = end_offset; diff --git a/src/interp/binary-reader-interp.cc b/src/interp/binary-reader-interp.cc index f4d05a1907..dc7ec41037 100644 --- a/src/interp/binary-reader-interp.cc +++ b/src/interp/binary-reader-interp.cc @@ -147,6 +147,7 @@ class BinaryReaderInterp : public BinaryReaderNop { Result BeginFunctionBody(Index index, Offset size) override; Result OnLocalDeclCount(Index count) override; Result OnLocalDecl(Index decl_index, Index count, Type type) override; + Result EndLocalDecls() override; Result OnOpcode(Opcode Opcode) override; Result OnAtomicLoadExpr(Opcode opcode, @@ -849,17 +850,6 @@ Result BinaryReaderInterp::EndFunctionBody(Index index) { Result BinaryReaderInterp::OnLocalDeclCount(Index count) { local_decl_count_ = count; local_count_ = 0; - // Continuation of the implicit func label, used for exception handling. (See - // BeginFunctionBody.) - // We need the local count for this, so we must do it here. - // NOTE: we don't count the parameters, as they're not part of the frame. - func_->handlers.push_back(HandlerDesc{HandlerKind::Catch, - istream_.end(), - Istream::kInvalidOffset, - {}, - {Istream::kInvalidOffset}, - static_cast(local_decl_count_), - 0}); return Result::Ok; } @@ -870,10 +860,26 @@ Result BinaryReaderInterp::OnLocalDecl(Index decl_index, local_count_ += count; func_->locals.push_back(LocalDesc{type, count, local_count_}); + return Result::Ok; +} - if (decl_index == local_decl_count_ - 1) { +Result BinaryReaderInterp::EndLocalDecls() { + if (local_count_ != 0) { istream_.Emit(Opcode::InterpAlloca, local_count_); } + // Continuation of the implicit func label, used for exception handling. (See + // BeginFunctionBody.) + // We need the local count for this, which is only available after processing + // all local decls. + // NOTE: we don't count the parameters, as they're not part of the frame. + func_->handlers.push_back(HandlerDesc{HandlerKind::Catch, + istream_.end(), + Istream::kInvalidOffset, + {}, + {Istream::kInvalidOffset}, + static_cast(local_count_), + 0}); + return Result::Ok; } @@ -1522,7 +1528,7 @@ Result BinaryReaderInterp::OnTryExpr(Type sig_type) { validator_.GetCatchCount(label_stack_.size() - 1, &exn_stack_height)); // NOTE: *NOT* GetLocalCount. we don't count the parameters, as they're not // part of the frame. - u32 value_stack_height = validator_.type_stack_size() + local_decl_count_; + u32 value_stack_height = validator_.type_stack_size() + local_count_; CHECK_RESULT(validator_.OnTry(GetLocation(), sig_type)); // Push a label that tracks mapping of exn -> catch PushLabel(LabelKind::Try, Istream::kInvalidOffset, Istream::kInvalidOffset, diff --git a/test/interp/basic-logging.txt b/test/interp/basic-logging.txt index d175d1c4b6..f62baea68c 100644 --- a/test/interp/basic-logging.txt +++ b/test/interp/basic-logging.txt @@ -62,6 +62,7 @@ BeginModule(version: 1) OnFunctionBodyCount(1) BeginFunctionBody(0, size:5) OnLocalDeclCount(0) + EndLocalDecls OnI32ConstExpr(42 (0x2a)) OnReturnExpr OnEndExpr From 3773a009c4916f24982ad6dfe149c1ce69a33059 Mon Sep 17 00:00:00 2001 From: SoniEx2 Date: Sat, 5 Oct 2024 09:52:40 -0300 Subject: [PATCH 2/3] Add a test, why not --- .../regress/interp-try-catch-multi-locals.txt | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 test/regress/interp-try-catch-multi-locals.txt diff --git a/test/regress/interp-try-catch-multi-locals.txt b/test/regress/interp-try-catch-multi-locals.txt new file mode 100644 index 0000000000..df1c365308 --- /dev/null +++ b/test/regress/interp-try-catch-multi-locals.txt @@ -0,0 +1,22 @@ +;;; TOOL: run-interp-spec +;;; ARGS*: --enable-exceptions +;;; NOTE: ref: issue-2476 +(module + (tag $e0) + (func (export "test") (result i32) + (local $a i32) + (local $b i32) + (try $try + (do + (throw $e0) + ) + (catch $e0) + ) + (local.get $a) + ) +) + +(assert_return (invoke "test") (i32.const 0)) +(;; STDOUT ;;; +2/2 tests passed. +;;; STDOUT ;;) From c97d2e1e9b59c52383ab7c51e5e48ce29a27a47d Mon Sep 17 00:00:00 2001 From: SoniEx2 Date: Mon, 7 Oct 2024 19:21:16 -0300 Subject: [PATCH 3/3] Merge tests related to interp EHv3 locals --- test/regress/interp-ehv3-locals.txt | 18 ++++++++++++--- .../regress/interp-try-catch-multi-locals.txt | 22 ------------------- 2 files changed, 15 insertions(+), 25 deletions(-) delete mode 100644 test/regress/interp-try-catch-multi-locals.txt diff --git a/test/regress/interp-ehv3-locals.txt b/test/regress/interp-ehv3-locals.txt index e4f05c830b..316563ab18 100644 --- a/test/regress/interp-ehv3-locals.txt +++ b/test/regress/interp-ehv3-locals.txt @@ -3,7 +3,7 @@ ;;; NOTE: ref: issue-2476 (module (tag $e0) - (func (export "broken-local") (result i32) + (func (export "set-local") (result i32) (local $value i32) (try $try (do @@ -14,9 +14,21 @@ ) (local.get $value) ) + (func (export "multiple-locals") (result i32) + (local $a i32) + (local $b i32) + (try $try + (do + (throw $e0) + ) + (catch $e0) + ) + (local.get $a) + ) ) -(assert_return (invoke "broken-local") (i32.const 1)) +(assert_return (invoke "set-local") (i32.const 1)) +(assert_return (invoke "multiple-locals") (i32.const 0)) (;; STDOUT ;;; -2/2 tests passed. +3/3 tests passed. ;;; STDOUT ;;) diff --git a/test/regress/interp-try-catch-multi-locals.txt b/test/regress/interp-try-catch-multi-locals.txt deleted file mode 100644 index df1c365308..0000000000 --- a/test/regress/interp-try-catch-multi-locals.txt +++ /dev/null @@ -1,22 +0,0 @@ -;;; TOOL: run-interp-spec -;;; ARGS*: --enable-exceptions -;;; NOTE: ref: issue-2476 -(module - (tag $e0) - (func (export "test") (result i32) - (local $a i32) - (local $b i32) - (try $try - (do - (throw $e0) - ) - (catch $e0) - ) - (local.get $a) - ) -) - -(assert_return (invoke "test") (i32.const 0)) -(;; STDOUT ;;; -2/2 tests passed. -;;; STDOUT ;;)