From e4305998cbcae7e301a2f69ce865d5096361724a Mon Sep 17 00:00:00 2001 From: KKiiim Date: Wed, 26 Aug 2026 14:23:37 +0800 Subject: [PATCH 1/5] fix review --- src/interp/binary-reader-interp.cc | 33 +++++++++---------- .../return-call-indirect-stack-cleanup.txt | 23 +++++++++++++ test/interp/return-call-stack-cleanup.txt | 21 ++++++++++++ 3 files changed, 59 insertions(+), 18 deletions(-) create mode 100644 test/interp/return-call-indirect-stack-cleanup.txt create mode 100644 test/interp/return-call-stack-cleanup.txt diff --git a/src/interp/binary-reader-interp.cc b/src/interp/binary-reader-interp.cc index 02d8f476e..369936a53 100644 --- a/src/interp/binary-reader-interp.cc +++ b/src/interp/binary-reader-interp.cc @@ -318,7 +318,7 @@ class BinaryReaderInterp : public BinaryReaderNop { Index* out_drop_count, Index* out_keep_count); Result GetReturnDropKeepCount(Index* out_drop_count, Index* out_keep_count); - Result GetReturnCallDropKeepCount(const FuncType&, + Result GetReturnCallDropKeepCount(Index param_count, Index keep_extra, Index* out_drop_count, Index* out_keep_count); @@ -456,11 +456,11 @@ Result BinaryReaderInterp::GetReturnDropKeepCount(Index* out_drop_count, return Result::Ok; } -Result BinaryReaderInterp::GetReturnCallDropKeepCount(const FuncType& func_type, +Result BinaryReaderInterp::GetReturnCallDropKeepCount(Index param_count, Index keep_extra, Index* out_drop_count, Index* out_keep_count) { - Index keep_count = static_cast(func_type.params.size()) + keep_extra; + Index keep_count = param_count + keep_extra; CHECK_RESULT(GetDropCount(keep_count, 0, out_drop_count)); *out_drop_count += validator_.GetLocalCount(); *out_keep_count = keep_count; @@ -1236,18 +1236,17 @@ Result BinaryReaderInterp::OnCallRefExpr(Type sig_type) { } Result BinaryReaderInterp::OnReturnCallExpr(Index func_index) { - CHECK_RESULT( - validator_.OnReturnCall(GetLocation(), Var(func_index, GetLocation()))); - - FuncType& func_type = func_types_[func_index]; + // Validation below rejects an invalid index before cleanup is emitted. + Index param_count = func_index < func_types_.size() + ? func_types_[func_index].params.size() + : 0; Index drop_count, keep_count, catch_drop_count; + // Validation consumes the tail-call operands, so compute cleanup first. CHECK_RESULT( - GetReturnCallDropKeepCount(func_type, 0, &drop_count, &keep_count)); + GetReturnCallDropKeepCount(param_count, 0, &drop_count, &keep_count)); CHECK_RESULT( validator_.GetCatchCount(label_stack_.size() - 1, &catch_drop_count)); - // The validator must be run after we get the drop/keep counts, since it - // will change the type stack. CHECK_RESULT( validator_.OnReturnCall(GetLocation(), Var(func_index, GetLocation()))); istream_.EmitDropKeep(drop_count, keep_count); @@ -1269,20 +1268,18 @@ Result BinaryReaderInterp::OnReturnCallExpr(Index func_index) { Result BinaryReaderInterp::OnReturnCallIndirectExpr(Index sig_index, Index table_index) { - CHECK_RESULT(validator_.OnReturnCallIndirect( - GetLocation(), Var(sig_index, GetLocation()), - Var(table_index, GetLocation()))); - - FuncType& func_type = module_.func_types[sig_index]; + // Validation below rejects an invalid index before cleanup is emitted. + Index param_count = sig_index < module_.func_types.size() + ? module_.func_types[sig_index].params.size() + : 0; Index drop_count, keep_count, catch_drop_count; // +1 to include the index of the function. + // Validation consumes the tail-call operands, so compute cleanup first. CHECK_RESULT( - GetReturnCallDropKeepCount(func_type, +1, &drop_count, &keep_count)); + GetReturnCallDropKeepCount(param_count, +1, &drop_count, &keep_count)); CHECK_RESULT( validator_.GetCatchCount(label_stack_.size() - 1, &catch_drop_count)); - // The validator must be run after we get the drop/keep counts, since it - // changes the type stack. CHECK_RESULT(validator_.OnReturnCallIndirect( GetLocation(), Var(sig_index, GetLocation()), Var(table_index, GetLocation()))); diff --git a/test/interp/return-call-indirect-stack-cleanup.txt b/test/interp/return-call-indirect-stack-cleanup.txt new file mode 100644 index 000000000..6a4d7cf01 --- /dev/null +++ b/test/interp/return-call-indirect-stack-cleanup.txt @@ -0,0 +1,23 @@ +;;; TOOL: run-interp +;;; ARGS1: --host-print +(module + (type $i_i (func (param i32) (result i32))) + (import "host" "print" (func $imported (type $i_i))) + (table funcref (elem $imported)) + + (func $tail (result i32) + i32.const 777 + i32.const 0 + i32.const 0 + return_call_indirect (type $i_i)) + + (func (export "f") (result i32) + (local i32) + call $tail + local.get 0 + return) +) +(;; STDOUT ;;; +called host host.print(i32:0) => i32:0 +f() => i32:0 +;;; STDOUT ;;) \ No newline at end of file diff --git a/test/interp/return-call-stack-cleanup.txt b/test/interp/return-call-stack-cleanup.txt new file mode 100644 index 000000000..4eb2d32ff --- /dev/null +++ b/test/interp/return-call-stack-cleanup.txt @@ -0,0 +1,21 @@ +;;; TOOL: run-interp +;;; ARGS1: --host-print +(module + (type $i_i (func (param i32) (result i32))) + (import "host" "print" (func $imported (type $i_i))) + + (func $tail (result i32) + i32.const 777 + i32.const 0 + return_call $imported) + + (func (export "f") (result i32) + (local i32) + call $tail + local.get 0 + return) +) +(;; STDOUT ;;; +called host host.print(i32:0) => i32:0 +f() => i32:0 +;;; STDOUT ;;) \ No newline at end of file From e7a212616351713a8cf83e1b86e7193e74e8e88e Mon Sep 17 00:00:00 2001 From: KKiiim Date: Mon, 31 Aug 2026 14:09:34 +0800 Subject: [PATCH 2/5] fix review --- src/interp/binary-reader-interp.cc | 35 +++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/src/interp/binary-reader-interp.cc b/src/interp/binary-reader-interp.cc index 369936a53..63c8a7fdf 100644 --- a/src/interp/binary-reader-interp.cc +++ b/src/interp/binary-reader-interp.cc @@ -1236,10 +1236,17 @@ Result BinaryReaderInterp::OnCallRefExpr(Type sig_type) { } Result BinaryReaderInterp::OnReturnCallExpr(Index func_index) { - // Validation below rejects an invalid index before cleanup is emitted. - Index param_count = func_index < func_types_.size() - ? func_types_[func_index].params.size() - : 0; + // Don't use OnReturnCall to check the index here: it consumes the call + // operands from the type stack. That will cause drop/keep counts to be + // incorrect. + if (func_index >= func_types_.size()) { + validator_.PrintError(GetLocation(), + "function variable out of range: %" PRIindex + " (max %" PRIindex ")", + func_index, static_cast(func_types_.size())); + return Result::Error; + } + Index param_count = func_types_[func_index].params.size(); Index drop_count, keep_count, catch_drop_count; // Validation consumes the tail-call operands, so compute cleanup first. @@ -1247,6 +1254,8 @@ Result BinaryReaderInterp::OnReturnCallExpr(Index func_index) { GetReturnCallDropKeepCount(param_count, 0, &drop_count, &keep_count)); CHECK_RESULT( validator_.GetCatchCount(label_stack_.size() - 1, &catch_drop_count)); + // The validator must be run after we get the drop/keep counts, since it + // will change the type stack. CHECK_RESULT( validator_.OnReturnCall(GetLocation(), Var(func_index, GetLocation()))); istream_.EmitDropKeep(drop_count, keep_count); @@ -1268,10 +1277,18 @@ Result BinaryReaderInterp::OnReturnCallExpr(Index func_index) { Result BinaryReaderInterp::OnReturnCallIndirectExpr(Index sig_index, Index table_index) { - // Validation below rejects an invalid index before cleanup is emitted. - Index param_count = sig_index < module_.func_types.size() - ? module_.func_types[sig_index].params.size() - : 0; + // Don't use OnReturnCallIndirect to check the index here: it consumes the + // call operands from the type stack. That will cause drop/keep counts to be + // incorrect. + if (sig_index >= module_.func_types.size()) { + validator_.PrintError(GetLocation(), + "function type variable out of range: %" PRIindex + " (max %" PRIindex ")", + sig_index, + static_cast(module_.func_types.size())); + return Result::Error; + } + Index param_count = module_.func_types[sig_index].params.size(); Index drop_count, keep_count, catch_drop_count; // +1 to include the index of the function. @@ -1280,6 +1297,8 @@ Result BinaryReaderInterp::OnReturnCallIndirectExpr(Index sig_index, GetReturnCallDropKeepCount(param_count, +1, &drop_count, &keep_count)); CHECK_RESULT( validator_.GetCatchCount(label_stack_.size() - 1, &catch_drop_count)); + // The validator must be run after we get the drop/keep counts, since it + // will change the type stack. CHECK_RESULT(validator_.OnReturnCallIndirect( GetLocation(), Var(sig_index, GetLocation()), Var(table_index, GetLocation()))); From 84bdf18fa671b6967aa1fb531ba19481228fa795 Mon Sep 17 00:00:00 2001 From: KKiiim Date: Tue, 1 Sep 2026 15:52:33 +0800 Subject: [PATCH 3/5] fix review --- src/interp/binary-reader-interp.cc | 48 +++++++++++++----------------- 1 file changed, 21 insertions(+), 27 deletions(-) diff --git a/src/interp/binary-reader-interp.cc b/src/interp/binary-reader-interp.cc index 63c8a7fdf..b23d0b5b3 100644 --- a/src/interp/binary-reader-interp.cc +++ b/src/interp/binary-reader-interp.cc @@ -318,7 +318,7 @@ class BinaryReaderInterp : public BinaryReaderNop { Index* out_drop_count, Index* out_keep_count); Result GetReturnDropKeepCount(Index* out_drop_count, Index* out_keep_count); - Result GetReturnCallDropKeepCount(Index param_count, + Result GetReturnCallDropKeepCount(const FuncType&, Index keep_extra, Index* out_drop_count, Index* out_keep_count); @@ -456,11 +456,11 @@ Result BinaryReaderInterp::GetReturnDropKeepCount(Index* out_drop_count, return Result::Ok; } -Result BinaryReaderInterp::GetReturnCallDropKeepCount(Index param_count, +Result BinaryReaderInterp::GetReturnCallDropKeepCount(const FuncType& func_type, Index keep_extra, Index* out_drop_count, Index* out_keep_count) { - Index keep_count = param_count + keep_extra; + Index keep_count = static_cast(func_type.params.size()) + keep_extra; CHECK_RESULT(GetDropCount(keep_count, 0, out_drop_count)); *out_drop_count += validator_.GetLocalCount(); *out_keep_count = keep_count; @@ -1236,22 +1236,19 @@ Result BinaryReaderInterp::OnCallRefExpr(Type sig_type) { } Result BinaryReaderInterp::OnReturnCallExpr(Index func_index) { - // Don't use OnReturnCall to check the index here: it consumes the call - // operands from the type stack. That will cause drop/keep counts to be - // incorrect. + // Do not unconditionally call OnReturnCall here: it consumes the call operands + // from the type stack, which would make subsequent drop/keep count calculations + // incorrect. We only call it here to report a standard validation error and bail + // out when the function index is out of bounds. if (func_index >= func_types_.size()) { - validator_.PrintError(GetLocation(), - "function variable out of range: %" PRIindex - " (max %" PRIindex ")", - func_index, static_cast(func_types_.size())); - return Result::Error; + return validator_.OnReturnCall(GetLocation(), + Var(func_index, GetLocation())); } - Index param_count = func_types_[func_index].params.size(); + FuncType& func_type = func_types_[func_index]; Index drop_count, keep_count, catch_drop_count; - // Validation consumes the tail-call operands, so compute cleanup first. CHECK_RESULT( - GetReturnCallDropKeepCount(param_count, 0, &drop_count, &keep_count)); + GetReturnCallDropKeepCount(func_type, 0, &drop_count, &keep_count)); CHECK_RESULT( validator_.GetCatchCount(label_stack_.size() - 1, &catch_drop_count)); // The validator must be run after we get the drop/keep counts, since it @@ -1277,28 +1274,25 @@ Result BinaryReaderInterp::OnReturnCallExpr(Index func_index) { Result BinaryReaderInterp::OnReturnCallIndirectExpr(Index sig_index, Index table_index) { - // Don't use OnReturnCallIndirect to check the index here: it consumes the - // call operands from the type stack. That will cause drop/keep counts to be - // incorrect. + // Do not unconditionally call OnReturnCallIndirect here: it consumes the call + // operands from the type stack, which would make subsequent drop/keep count + // calculations incorrect. We only call it here to report a standard validation + // error and bail out when the function type index is out of bounds. if (sig_index >= module_.func_types.size()) { - validator_.PrintError(GetLocation(), - "function type variable out of range: %" PRIindex - " (max %" PRIindex ")", - sig_index, - static_cast(module_.func_types.size())); - return Result::Error; + return validator_.OnReturnCallIndirect( + GetLocation(), Var(sig_index, GetLocation()), + Var(table_index, GetLocation())); } - Index param_count = module_.func_types[sig_index].params.size(); + FuncType& func_type = module_.func_types[sig_index]; Index drop_count, keep_count, catch_drop_count; // +1 to include the index of the function. - // Validation consumes the tail-call operands, so compute cleanup first. CHECK_RESULT( - GetReturnCallDropKeepCount(param_count, +1, &drop_count, &keep_count)); + GetReturnCallDropKeepCount(func_type, +1, &drop_count, &keep_count)); CHECK_RESULT( validator_.GetCatchCount(label_stack_.size() - 1, &catch_drop_count)); // The validator must be run after we get the drop/keep counts, since it - // will change the type stack. + // changes the type stack. CHECK_RESULT(validator_.OnReturnCallIndirect( GetLocation(), Var(sig_index, GetLocation()), Var(table_index, GetLocation()))); From b7c67324918c99cef9dc07ba0a1edd0dfa6c7b09 Mon Sep 17 00:00:00 2001 From: KKiiim Date: Tue, 1 Sep 2026 16:01:44 +0800 Subject: [PATCH 4/5] linter --- src/interp/binary-reader-interp.cc | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/interp/binary-reader-interp.cc b/src/interp/binary-reader-interp.cc index b23d0b5b3..da8253496 100644 --- a/src/interp/binary-reader-interp.cc +++ b/src/interp/binary-reader-interp.cc @@ -1236,10 +1236,10 @@ Result BinaryReaderInterp::OnCallRefExpr(Type sig_type) { } Result BinaryReaderInterp::OnReturnCallExpr(Index func_index) { - // Do not unconditionally call OnReturnCall here: it consumes the call operands - // from the type stack, which would make subsequent drop/keep count calculations - // incorrect. We only call it here to report a standard validation error and bail - // out when the function index is out of bounds. + // Do not unconditionally call OnReturnCall here: it consumes the call + // operands from the type stack, which would make subsequent drop/keep count + // calculations incorrect. We only call it here to report a standard + // validation error and bail out when the function index is out of bounds. if (func_index >= func_types_.size()) { return validator_.OnReturnCall(GetLocation(), Var(func_index, GetLocation())); @@ -1276,12 +1276,13 @@ Result BinaryReaderInterp::OnReturnCallIndirectExpr(Index sig_index, Index table_index) { // Do not unconditionally call OnReturnCallIndirect here: it consumes the call // operands from the type stack, which would make subsequent drop/keep count - // calculations incorrect. We only call it here to report a standard validation - // error and bail out when the function type index is out of bounds. + // calculations incorrect. We only call it here to report a standard + // validation error and bail out when the function type index is out of + // bounds. if (sig_index >= module_.func_types.size()) { - return validator_.OnReturnCallIndirect( - GetLocation(), Var(sig_index, GetLocation()), - Var(table_index, GetLocation())); + return validator_.OnReturnCallIndirect(GetLocation(), + Var(sig_index, GetLocation()), + Var(table_index, GetLocation())); } FuncType& func_type = module_.func_types[sig_index]; From 87a85f043c931834aa9a59e005be0156c5526003 Mon Sep 17 00:00:00 2001 From: KKiiim Date: Fri, 4 Sep 2026 15:29:05 +0800 Subject: [PATCH 5/5] fix --- src/interp/binary-reader-interp.cc | 63 ++++++++++++------------------ 1 file changed, 26 insertions(+), 37 deletions(-) diff --git a/src/interp/binary-reader-interp.cc b/src/interp/binary-reader-interp.cc index da8253496..32abb4256 100644 --- a/src/interp/binary-reader-interp.cc +++ b/src/interp/binary-reader-interp.cc @@ -319,6 +319,7 @@ class BinaryReaderInterp : public BinaryReaderNop { Index* out_keep_count); Result GetReturnDropKeepCount(Index* out_drop_count, Index* out_keep_count); Result GetReturnCallDropKeepCount(const FuncType&, + size_t orig_type_stack_size, Index keep_extra, Index* out_drop_count, Index* out_keep_count); @@ -456,13 +457,17 @@ Result BinaryReaderInterp::GetReturnDropKeepCount(Index* out_drop_count, return Result::Ok; } -Result BinaryReaderInterp::GetReturnCallDropKeepCount(const FuncType& func_type, - Index keep_extra, - Index* out_drop_count, - Index* out_keep_count) { +Result BinaryReaderInterp::GetReturnCallDropKeepCount( + const FuncType& func_type, + size_t orig_type_stack_size, + Index keep_extra, + Index* out_drop_count, + Index* out_keep_count) { Index keep_count = static_cast(func_type.params.size()) + keep_extra; - CHECK_RESULT(GetDropCount(keep_count, 0, out_drop_count)); - *out_drop_count += validator_.GetLocalCount(); + *out_drop_count = + (orig_type_stack_size >= keep_count ? orig_type_stack_size - keep_count + : 0) + + validator_.GetLocalCount(); *out_keep_count = keep_count; return Result::Ok; } @@ -1236,25 +1241,18 @@ Result BinaryReaderInterp::OnCallRefExpr(Type sig_type) { } Result BinaryReaderInterp::OnReturnCallExpr(Index func_index) { - // Do not unconditionally call OnReturnCall here: it consumes the call - // operands from the type stack, which would make subsequent drop/keep count - // calculations incorrect. We only call it here to report a standard - // validation error and bail out when the function index is out of bounds. - if (func_index >= func_types_.size()) { - return validator_.OnReturnCall(GetLocation(), - Var(func_index, GetLocation())); - } + // Capture type stack size before validation sets the block to unreachable. + size_t orig_type_stack_size = validator_.type_stack_size(); + CHECK_RESULT( + validator_.OnReturnCall(GetLocation(), Var(func_index, GetLocation()))); + FuncType& func_type = func_types_[func_index]; Index drop_count, keep_count, catch_drop_count; - CHECK_RESULT( - GetReturnCallDropKeepCount(func_type, 0, &drop_count, &keep_count)); + CHECK_RESULT(GetReturnCallDropKeepCount(func_type, orig_type_stack_size, 0, + &drop_count, &keep_count)); CHECK_RESULT( validator_.GetCatchCount(label_stack_.size() - 1, &catch_drop_count)); - // The validator must be run after we get the drop/keep counts, since it - // will change the type stack. - CHECK_RESULT( - validator_.OnReturnCall(GetLocation(), Var(func_index, GetLocation()))); istream_.EmitDropKeep(drop_count, keep_count); istream_.EmitCatchDrop(catch_drop_count); @@ -1274,29 +1272,20 @@ Result BinaryReaderInterp::OnReturnCallExpr(Index func_index) { Result BinaryReaderInterp::OnReturnCallIndirectExpr(Index sig_index, Index table_index) { - // Do not unconditionally call OnReturnCallIndirect here: it consumes the call - // operands from the type stack, which would make subsequent drop/keep count - // calculations incorrect. We only call it here to report a standard - // validation error and bail out when the function type index is out of - // bounds. - if (sig_index >= module_.func_types.size()) { - return validator_.OnReturnCallIndirect(GetLocation(), - Var(sig_index, GetLocation()), - Var(table_index, GetLocation())); - } + // Capture type stack size before validation sets the block to unreachable. + size_t orig_type_stack_size = validator_.type_stack_size(); + CHECK_RESULT(validator_.OnReturnCallIndirect( + GetLocation(), Var(sig_index, GetLocation()), + Var(table_index, GetLocation()))); + FuncType& func_type = module_.func_types[sig_index]; Index drop_count, keep_count, catch_drop_count; // +1 to include the index of the function. - CHECK_RESULT( - GetReturnCallDropKeepCount(func_type, +1, &drop_count, &keep_count)); + CHECK_RESULT(GetReturnCallDropKeepCount(func_type, orig_type_stack_size, +1, + &drop_count, &keep_count)); CHECK_RESULT( validator_.GetCatchCount(label_stack_.size() - 1, &catch_drop_count)); - // The validator must be run after we get the drop/keep counts, since it - // changes the type stack. - CHECK_RESULT(validator_.OnReturnCallIndirect( - GetLocation(), Var(sig_index, GetLocation()), - Var(table_index, GetLocation()))); istream_.EmitDropKeep(drop_count, keep_count); istream_.EmitCatchDrop(catch_drop_count); istream_.Emit(Opcode::ReturnCallIndirect, table_index, sig_index);