From 40e51faca897e3952a0002ae4e1459f08d808cdc Mon Sep 17 00:00:00 2001 From: Xu Jun Date: Thu, 1 Feb 2024 11:55:29 +0800 Subject: [PATCH] fast-interp: Fix block with parameter in polymorphic stack issue (#3112) The issue was reported in https://github.com/bytecodealliance/wasm-micro-runtime/issues/3061. --- core/iwasm/interpreter/wasm_loader.c | 146 ++++++++++------------ core/iwasm/interpreter/wasm_mini_loader.c | 143 ++++++++++----------- 2 files changed, 130 insertions(+), 159 deletions(-) diff --git a/core/iwasm/interpreter/wasm_loader.c b/core/iwasm/interpreter/wasm_loader.c index 520ca8091d..7b3eb365cd 100644 --- a/core/iwasm/interpreter/wasm_loader.c +++ b/core/iwasm/interpreter/wasm_loader.c @@ -5280,9 +5280,8 @@ typedef struct BranchBlock { BranchBlockPatch *patch_list; /* This is used to save params frame_offset of of if block */ int16 *param_frame_offsets; - /* This is used to store available param num for if/else branch, so the else - * opcode can know how many parameters should be copied to the stack */ - uint32 available_param_num; + /* This is used to recover dynamic offset for else branch */ + uint16 start_dynamic_offset; #endif /* Indicate the operand stack is in polymorphic state. @@ -7219,18 +7218,15 @@ check_block_stack(WASMLoaderContext *loader_ctx, BranchBlock *block, * 1) POP original parameter out; * 2) Push and copy original values to dynamic space. * The copy instruction format: - * Part a: available param count + * Part a: param count * Part b: all param total cell num * Part c: each param's cell_num, src offset and dst offset * Part d: each param's src offset * Part e: each param's dst offset - * Note: if the stack is in polymorphic state, the actual copied parameters may - * be fewer than the defined number in block type */ static bool copy_params_to_dynamic_space(WASMLoaderContext *loader_ctx, bool is_if_block, - uint32 *p_available_param_count, char *error_buf, - uint32 error_buf_size) + char *error_buf, uint32 error_buf_size) { bool ret = false; int16 *frame_offset = NULL; @@ -7242,91 +7238,72 @@ copy_params_to_dynamic_space(WASMLoaderContext *loader_ctx, bool is_if_block, BlockType *block_type = &block->block_type; WASMType *wasm_type = block_type->u.type; uint32 param_count = block_type->u.type->param_count; - uint32 available_param_count = 0; int16 condition_offset = 0; bool disable_emit = false; int16 operand_offset = 0; - uint64 size; - - if (is_if_block) - condition_offset = *loader_ctx->frame_offset; - - /* POP original parameter out */ - for (i = 0; i < param_count; i++) { - int32 available_stack_cell = - (int32)(loader_ctx->stack_cell_num - block->stack_cell_num); - - if (available_stack_cell <= 0 && block->is_stack_polymorphic) - break; - - POP_OFFSET_TYPE(wasm_type->types[param_count - i - 1]); - wasm_loader_emit_backspace(loader_ctx, sizeof(int16)); - } - available_param_count = i; - size = - (uint64)available_param_count * (sizeof(*cells) + sizeof(*src_offsets)); + uint64 size = (uint64)param_count * (sizeof(*cells) + sizeof(*src_offsets)); + bh_assert(size > 0); /* For if block, we also need copy the condition operand offset. */ if (is_if_block) size += sizeof(*cells) + sizeof(*src_offsets); /* Allocate memory for the emit data */ - if ((size > 0) - && !(emit_data = loader_malloc(size, error_buf, error_buf_size))) + if (!(emit_data = loader_malloc(size, error_buf, error_buf_size))) return false; cells = emit_data; src_offsets = (int16 *)(cells + param_count); + if (is_if_block) + condition_offset = *loader_ctx->frame_offset; + + /* POP original parameter out */ + for (i = 0; i < param_count; i++) { + POP_OFFSET_TYPE(wasm_type->types[param_count - i - 1]); + wasm_loader_emit_backspace(loader_ctx, sizeof(int16)); + } frame_offset = loader_ctx->frame_offset; /* Get each param's cell num and src offset */ - for (i = 0; i < available_param_count; i++) { + for (i = 0; i < param_count; i++) { cell = (uint8)wasm_value_type_cell_num(wasm_type->types[i]); cells[i] = cell; src_offsets[i] = *frame_offset; frame_offset += cell; } - /* emit copy instruction */ emit_label(EXT_OP_COPY_STACK_VALUES); /* Part a) */ - emit_uint32(loader_ctx, is_if_block ? available_param_count + 1 - : available_param_count); + emit_uint32(loader_ctx, is_if_block ? param_count + 1 : param_count); /* Part b) */ emit_uint32(loader_ctx, is_if_block ? wasm_type->param_cell_num + 1 : wasm_type->param_cell_num); /* Part c) */ - for (i = 0; i < available_param_count; i++) + for (i = 0; i < param_count; i++) emit_byte(loader_ctx, cells[i]); if (is_if_block) emit_byte(loader_ctx, 1); /* Part d) */ - for (i = 0; i < available_param_count; i++) + for (i = 0; i < param_count; i++) emit_operand(loader_ctx, src_offsets[i]); if (is_if_block) emit_operand(loader_ctx, condition_offset); /* Part e) */ /* Push to dynamic space. The push will emit the dst offset. */ - for (i = 0; i < available_param_count; i++) + for (i = 0; i < param_count; i++) PUSH_OFFSET_TYPE(wasm_type->types[i]); if (is_if_block) PUSH_OFFSET_TYPE(VALUE_TYPE_I32); - if (p_available_param_count) { - *p_available_param_count = available_param_count; - } - ret = true; fail: /* Free the emit data */ - if (emit_data) { - wasm_runtime_free(emit_data); - } + wasm_runtime_free(emit_data); return ret; } @@ -7616,6 +7593,7 @@ wasm_loader_prepare_bytecode(WASMModule *module, WASMFunction *func, BranchBlock *cur_block = loader_ctx->frame_csp - 1; #if WASM_ENABLE_FAST_INTERP != 0 + uint32 cell_num; available_params = block_type.u.type->param_count; #endif for (i = 0; i < block_type.u.type->param_count; i++) { @@ -7633,6 +7611,13 @@ wasm_loader_prepare_bytecode(WASMModule *module, WASMFunction *func, POP_TYPE( wasm_type->types[wasm_type->param_count - i - 1]); +#if WASM_ENABLE_FAST_INTERP != 0 + /* decrease the frame_offset pointer accordingly to keep + * consistent with frame_ref stack */ + cell_num = wasm_value_type_cell_num( + wasm_type->types[wasm_type->param_count - i - 1]); + loader_ctx->frame_offset -= cell_num; +#endif } } PUSH_CSP(LABEL_TYPE_BLOCK + (opcode - WASM_OP_BLOCK), @@ -7641,12 +7626,26 @@ wasm_loader_prepare_bytecode(WASMModule *module, WASMFunction *func, /* Pass parameters to block */ if (BLOCK_HAS_PARAM(block_type)) { for (i = 0; i < block_type.u.type->param_count; i++) { - PUSH_TYPE(block_type.u.type->types[i]); #if WASM_ENABLE_FAST_INTERP != 0 + uint32 cell_num = wasm_value_type_cell_num( + block_type.u.type->types[i]); if (i >= available_params) { - PUSH_OFFSET_TYPE(block_type.u.type->types[i]); + /* If there isn't enough data on stack, push a dummy + * offset to keep the stack consistent with + * frame_ref. + * Since the stack is already in polymorphic state, + * the opcode will not be executed, so the dummy + * offset won't cause any error */ + *loader_ctx->frame_offset++ = 0; + if (cell_num > 1) { + *loader_ctx->frame_offset++ = 0; + } + } + else { + loader_ctx->frame_offset += cell_num; } #endif + PUSH_TYPE(block_type.u.type->types[i]); } } @@ -7656,9 +7655,8 @@ wasm_loader_prepare_bytecode(WASMModule *module, WASMFunction *func, if (BLOCK_HAS_PARAM(block_type)) { /* Make sure params are in dynamic space */ - if (!copy_params_to_dynamic_space(loader_ctx, false, - NULL, error_buf, - error_buf_size)) + if (!copy_params_to_dynamic_space( + loader_ctx, false, error_buf, error_buf_size)) goto fail; } @@ -7691,17 +7689,21 @@ wasm_loader_prepare_bytecode(WASMModule *module, WASMFunction *func, * recover them before entering else branch. * */ - if (if_condition_available && BLOCK_HAS_PARAM(block_type)) { + if (BLOCK_HAS_PARAM(block_type)) { uint64 size; - /* skip the if condition operand offset */ - wasm_loader_emit_backspace(loader_ctx, sizeof(int16)); + /* In polymorphic state, there may be no if condition on + * the stack, so the offset may not emitted */ + if (if_condition_available) { + /* skip the if condition operand offset */ + wasm_loader_emit_backspace(loader_ctx, + sizeof(int16)); + } /* skip the if label */ skip_label(); /* Emit a copy instruction */ if (!copy_params_to_dynamic_space( - loader_ctx, true, &block->available_param_num, - error_buf, error_buf_size)) + loader_ctx, true, error_buf, error_buf_size)) goto fail; /* Emit the if instruction */ @@ -7722,9 +7724,8 @@ wasm_loader_prepare_bytecode(WASMModule *module, WASMFunction *func, - size / sizeof(int16), (uint32)size); } - else { - block->available_param_num = 0; - } + + block->start_dynamic_offset = loader_ctx->dynamic_offset; emit_empty_label_addr_and_frame_ip(PATCH_ELSE); emit_empty_label_addr_and_frame_ip(PATCH_END); @@ -7980,30 +7981,13 @@ wasm_loader_prepare_bytecode(WASMModule *module, WASMFunction *func, #if WASM_ENABLE_FAST_INTERP != 0 /* Recover top param_count values of frame_offset stack */ - if (block->available_param_num) { - uint32 available_param_cell_num = 0; - - /* total cell num of available parameters */ - for (i = 0; i < block->available_param_num; i++) { - uint32 cell_num = wasm_value_type_cell_num( - block->block_type.u.type->types[i]); - - /* recover dynamic offset */ - if (block->param_frame_offsets[available_param_cell_num] - >= loader_ctx->dynamic_offset) { - loader_ctx->dynamic_offset = - block->param_frame_offsets - [available_param_cell_num] - + cell_num; - } - - available_param_cell_num += cell_num; - } - - bh_memcpy_s( - loader_ctx->frame_offset, available_param_cell_num, - block->param_frame_offsets, available_param_cell_num); - loader_ctx->frame_offset += available_param_cell_num; + if (BLOCK_HAS_PARAM((block_type))) { + uint32 size; + size = sizeof(int16) * block_type.u.type->param_cell_num; + bh_memcpy_s(loader_ctx->frame_offset, size, + block->param_frame_offsets, size); + loader_ctx->frame_offset += (size / sizeof(int16)); + loader_ctx->dynamic_offset = block->start_dynamic_offset; } #endif diff --git a/core/iwasm/interpreter/wasm_mini_loader.c b/core/iwasm/interpreter/wasm_mini_loader.c index c617e08437..e283b10c91 100644 --- a/core/iwasm/interpreter/wasm_mini_loader.c +++ b/core/iwasm/interpreter/wasm_mini_loader.c @@ -3614,6 +3614,8 @@ typedef struct BranchBlock { /* This is used to store available param num for if/else branch, so the else * opcode can know how many parameters should be copied to the stack */ uint32 available_param_num; + /* This is used to recover dynamic offset for else branch */ + uint16 start_dynamic_offset; #endif /* Indicate the operand stack is in polymorphic state. @@ -5363,18 +5365,15 @@ check_block_stack(WASMLoaderContext *loader_ctx, BranchBlock *block, * 1) POP original parameter out; * 2) Push and copy original values to dynamic space. * The copy instruction format: - * Part a: available param count + * Part a: param count * Part b: all param total cell num * Part c: each param's cell_num, src offset and dst offset * Part d: each param's src offset * Part e: each param's dst offset - * Note: if the stack is in polymorphic state, the actual copied parameters may - * be fewer than the defined number in block type */ static bool copy_params_to_dynamic_space(WASMLoaderContext *loader_ctx, bool is_if_block, - uint32 *p_available_param_count, char *error_buf, - uint32 error_buf_size) + char *error_buf, uint32 error_buf_size) { bool ret = false; int16 *frame_offset = NULL; @@ -5386,91 +5385,72 @@ copy_params_to_dynamic_space(WASMLoaderContext *loader_ctx, bool is_if_block, BlockType *block_type = &block->block_type; WASMType *wasm_type = block_type->u.type; uint32 param_count = block_type->u.type->param_count; - uint32 available_param_count = 0; int16 condition_offset = 0; bool disable_emit = false; int16 operand_offset = 0; - uint64 size; - - if (is_if_block) - condition_offset = *loader_ctx->frame_offset; - - /* POP original parameter out */ - for (i = 0; i < param_count; i++) { - int32 available_stack_cell = - (int32)(loader_ctx->stack_cell_num - block->stack_cell_num); - - if (available_stack_cell <= 0 && block->is_stack_polymorphic) - break; - - POP_OFFSET_TYPE(wasm_type->types[param_count - i - 1]); - wasm_loader_emit_backspace(loader_ctx, sizeof(int16)); - } - available_param_count = i; - size = - (uint64)available_param_count * (sizeof(*cells) + sizeof(*src_offsets)); + uint64 size = (uint64)param_count * (sizeof(*cells) + sizeof(*src_offsets)); + bh_assert(size > 0); /* For if block, we also need copy the condition operand offset. */ if (is_if_block) size += sizeof(*cells) + sizeof(*src_offsets); /* Allocate memory for the emit data */ - if ((size > 0) - && !(emit_data = loader_malloc(size, error_buf, error_buf_size))) + if (!(emit_data = loader_malloc(size, error_buf, error_buf_size))) return false; cells = emit_data; src_offsets = (int16 *)(cells + param_count); + if (is_if_block) + condition_offset = *loader_ctx->frame_offset; + + /* POP original parameter out */ + for (i = 0; i < param_count; i++) { + POP_OFFSET_TYPE(wasm_type->types[param_count - i - 1]); + wasm_loader_emit_backspace(loader_ctx, sizeof(int16)); + } frame_offset = loader_ctx->frame_offset; /* Get each param's cell num and src offset */ - for (i = 0; i < available_param_count; i++) { + for (i = 0; i < param_count; i++) { cell = (uint8)wasm_value_type_cell_num(wasm_type->types[i]); cells[i] = cell; src_offsets[i] = *frame_offset; frame_offset += cell; } - /* emit copy instruction */ emit_label(EXT_OP_COPY_STACK_VALUES); /* Part a) */ - emit_uint32(loader_ctx, is_if_block ? available_param_count + 1 - : available_param_count); + emit_uint32(loader_ctx, is_if_block ? param_count + 1 : param_count); /* Part b) */ emit_uint32(loader_ctx, is_if_block ? wasm_type->param_cell_num + 1 : wasm_type->param_cell_num); /* Part c) */ - for (i = 0; i < available_param_count; i++) + for (i = 0; i < param_count; i++) emit_byte(loader_ctx, cells[i]); if (is_if_block) emit_byte(loader_ctx, 1); /* Part d) */ - for (i = 0; i < available_param_count; i++) + for (i = 0; i < param_count; i++) emit_operand(loader_ctx, src_offsets[i]); if (is_if_block) emit_operand(loader_ctx, condition_offset); /* Part e) */ /* Push to dynamic space. The push will emit the dst offset. */ - for (i = 0; i < available_param_count; i++) + for (i = 0; i < param_count; i++) PUSH_OFFSET_TYPE(wasm_type->types[i]); if (is_if_block) PUSH_OFFSET_TYPE(VALUE_TYPE_I32); - if (p_available_param_count) { - *p_available_param_count = available_param_count; - } - ret = true; fail: /* Free the emit data */ - if (emit_data) { - wasm_runtime_free(emit_data); - } + wasm_runtime_free(emit_data); return ret; } @@ -5672,6 +5652,7 @@ wasm_loader_prepare_bytecode(WASMModule *module, WASMFunction *func, BranchBlock *cur_block = loader_ctx->frame_csp - 1; #if WASM_ENABLE_FAST_INTERP != 0 + uint32 cell_num; available_params = block_type.u.type->param_count; #endif for (i = 0; i < block_type.u.type->param_count; i++) { @@ -5689,6 +5670,13 @@ wasm_loader_prepare_bytecode(WASMModule *module, WASMFunction *func, POP_TYPE( wasm_type->types[wasm_type->param_count - i - 1]); +#if WASM_ENABLE_FAST_INTERP != 0 + /* decrease the frame_offset pointer accordingly to keep + * consistent with frame_ref stack */ + cell_num = wasm_value_type_cell_num( + wasm_type->types[wasm_type->param_count - i - 1]); + loader_ctx->frame_offset -= cell_num; +#endif } } @@ -5698,12 +5686,26 @@ wasm_loader_prepare_bytecode(WASMModule *module, WASMFunction *func, /* Pass parameters to block */ if (BLOCK_HAS_PARAM(block_type)) { for (i = 0; i < block_type.u.type->param_count; i++) { - PUSH_TYPE(block_type.u.type->types[i]); #if WASM_ENABLE_FAST_INTERP != 0 + uint32 cell_num = wasm_value_type_cell_num( + block_type.u.type->types[i]); if (i >= available_params) { - PUSH_OFFSET_TYPE(block_type.u.type->types[i]); + /* If there isn't enough data on stack, push a dummy + * offset to keep the stack consistent with + * frame_ref. + * Since the stack is already in polymorphic state, + * the opcode will not be executed, so the dummy + * offset won't cause any error */ + *loader_ctx->frame_offset++ = 0; + if (cell_num > 1) { + *loader_ctx->frame_offset++ = 0; + } + } + else { + loader_ctx->frame_offset += cell_num; } #endif + PUSH_TYPE(block_type.u.type->types[i]); } } @@ -5712,9 +5714,8 @@ wasm_loader_prepare_bytecode(WASMModule *module, WASMFunction *func, skip_label(); if (BLOCK_HAS_PARAM(block_type)) { /* Make sure params are in dynamic space */ - if (!copy_params_to_dynamic_space(loader_ctx, false, - NULL, error_buf, - error_buf_size)) + if (!copy_params_to_dynamic_space( + loader_ctx, false, error_buf, error_buf_size)) goto fail; } if (opcode == WASM_OP_LOOP) { @@ -5741,17 +5742,21 @@ wasm_loader_prepare_bytecode(WASMModule *module, WASMFunction *func, * recover them before entering else branch. * */ - if (if_condition_available && BLOCK_HAS_PARAM(block_type)) { + if (BLOCK_HAS_PARAM(block_type)) { uint64 size; - /* skip the if condition operand offset */ - wasm_loader_emit_backspace(loader_ctx, sizeof(int16)); + /* In polymorphic state, there may be no if condition on + * the stack, so the offset may not emitted */ + if (if_condition_available) { + /* skip the if condition operand offset */ + wasm_loader_emit_backspace(loader_ctx, + sizeof(int16)); + } /* skip the if label */ skip_label(); /* Emit a copy instruction */ if (!copy_params_to_dynamic_space( - loader_ctx, true, &block->available_param_num, - error_buf, error_buf_size)) + loader_ctx, true, error_buf, error_buf_size)) goto fail; /* Emit the if instruction */ @@ -5772,9 +5777,8 @@ wasm_loader_prepare_bytecode(WASMModule *module, WASMFunction *func, - size / sizeof(int16), (uint32)size); } - else { - block->available_param_num = 0; - } + + block->start_dynamic_offset = loader_ctx->dynamic_offset; emit_empty_label_addr_and_frame_ip(PATCH_ELSE); emit_empty_label_addr_and_frame_ip(PATCH_END); @@ -5818,30 +5822,13 @@ wasm_loader_prepare_bytecode(WASMModule *module, WASMFunction *func, #if WASM_ENABLE_FAST_INTERP != 0 /* Recover top param_count values of frame_offset stack */ - if (block->available_param_num) { - uint32 available_param_cell_num = 0; - - /* total cell num of available parameters */ - for (i = 0; i < block->available_param_num; i++) { - uint32 cell_num = wasm_value_type_cell_num( - block->block_type.u.type->types[i]); - - /* recover dynamic offset */ - if (block->param_frame_offsets[available_param_cell_num] - >= loader_ctx->dynamic_offset) { - loader_ctx->dynamic_offset = - block->param_frame_offsets - [available_param_cell_num] - + cell_num; - } - - available_param_cell_num += cell_num; - } - - bh_memcpy_s( - loader_ctx->frame_offset, available_param_cell_num, - block->param_frame_offsets, available_param_cell_num); - loader_ctx->frame_offset += available_param_cell_num; + if (BLOCK_HAS_PARAM((block_type))) { + uint32 size; + size = sizeof(int16) * block_type.u.type->param_cell_num; + bh_memcpy_s(loader_ctx->frame_offset, size, + block->param_frame_offsets, size); + loader_ctx->frame_offset += (size / sizeof(int16)); + loader_ctx->dynamic_offset = block->start_dynamic_offset; } #endif