Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable aux stack allocations for threads spawned by wasi_thread_start #1867

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions core/iwasm/common/wasm_exec_env.h
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,13 @@ wasm_exec_env_create(struct WASMModuleInstanceCommon *module_inst,
void
wasm_exec_env_destroy(WASMExecEnv *exec_env);

static inline bool
wasm_exec_env_is_aux_stack_managed_by_runtime(WASMExecEnv *exec_env)
{
return exec_env->aux_stack_boundary.boundary != 0
|| exec_env->aux_stack_bottom.bottom != 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i feel it's simpler to use a dedicated flag as 0 is a valid address in wasm. how do you think?

also, i guess aot needs an equivalent around create_aux_stack_info.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i feel it's simpler to use a dedicated flag as 0 is a valid address in wasm. how do you think?

Yeah, I did think about it. But I noticed that WAMR actually doesn't always respect 0 as a valid address; for example, it considers wasm_runtime_malloc() returning 0 as a failure. Flag was another option but didn't want to increase the size of the object unnecessarily if there was a way to achieve the same without it (if my assumption about WAMR not respecting 0 as a correct value holds).

also, i guess aot needs an equivalent around create_aux_stack_info.

That's correct. At the moment we're not testing AOT though so I was planning to do that (and other necessary things) in the follow-up PR. Does it make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i feel it's simpler to use a dedicated flag as 0 is a valid address in wasm. how do you think?

Yeah, I did think about it. But I noticed that WAMR actually doesn't always respect 0 as a valid address; for example, it considers wasm_runtime_malloc() returning 0 as a failure. Flag was another option but didn't want to increase the size of the object unnecessarily if there was a way to achieve the same without it (if my assumption about WAMR not respecting 0 as a correct value holds).

ok.

also, i guess aot needs an equivalent around create_aux_stack_info.

That's correct. At the moment we're not testing AOT though so I was planning to do that (and other necessary things) in the follow-up PR. Does it make sense?

ok.

}

/**
* Allocate a WASM frame from the WASM stack.
*
Expand Down
20 changes: 12 additions & 8 deletions core/iwasm/interpreter/wasm_interp_classic.c
Original file line number Diff line number Diff line change
Expand Up @@ -1783,14 +1783,18 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module,
global = globals + global_idx;
global_addr = get_global_addr(global_data, global);
aux_stack_top = *(uint32 *)(frame_sp - 1);
if (aux_stack_top <= exec_env->aux_stack_boundary.boundary) {
wasm_set_exception(module, "wasm auxiliary stack overflow");
goto got_exception;
}
if (aux_stack_top > exec_env->aux_stack_bottom.bottom) {
wasm_set_exception(module,
"wasm auxiliary stack underflow");
goto got_exception;
if (wasm_exec_env_is_aux_stack_managed_by_runtime(exec_env)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we do; however, for now we focus on the interpreter and we'll work on JIT/AOT next.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so let's merge this PR first?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can merge this one and next we'll continue working on AOT and JIT

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wenyongh @yamt for AOT, should we disable the aux stack globally when WASI threads are enabled? by replacing this

comp_ctx->enable_aux_stack_check = true;

with something like

#if WASM_ENABLE_LIB_WASI_THREADS != 0
        comp_ctx->enable_aux_stack_check = false;
#else
        comp_ctx->enable_aux_stack_check = true;
#endif

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess it can be a workaround.
but i think it's better to avoid making the compiler depend on build-time configurations like this.

Copy link
Contributor

@eloparco eloparco Jan 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at this point, should we just specify in the documentation to compile with --disable-aux-stack-check when using WASI threads with AOT?
for fast JIT we could instead put an #if WASM_ENABLE_LIB_WASI_THREADS == 0 around here

if (is_aux_stack) {
JitReg aux_stack_bound = get_aux_stack_bound_reg(cc->jit_frame);
JitReg aux_stack_bottom =
get_aux_stack_bottom_reg(cc->jit_frame);
GEN_INSN(CMP, cc->cmp_reg, value, aux_stack_bound);
if (!(jit_emit_exception(cc, EXCE_AUX_STACK_OVERFLOW,
JIT_OP_BLEU, cc->cmp_reg, NULL)))
goto fail;
GEN_INSN(CMP, cc->cmp_reg, value, aux_stack_bottom);
if (!(jit_emit_exception(cc, EXCE_AUX_STACK_UNDERFLOW,
JIT_OP_BGTU, cc->cmp_reg, NULL)))
goto fail;
}
GEN_INSN(STI32, value, get_module_inst_reg(cc->jit_frame),
NEW_CONST(I32, data_offset));
break;
}

if (aux_stack_top
<= exec_env->aux_stack_boundary.boundary) {
wasm_set_exception(module,
"wasm auxiliary stack overflow");
goto got_exception;
}
if (aux_stack_top > exec_env->aux_stack_bottom.bottom) {
wasm_set_exception(module,
"wasm auxiliary stack underflow");
goto got_exception;
}
}
*(int32 *)global_addr = aux_stack_top;
frame_sp--;
Expand Down
20 changes: 12 additions & 8 deletions core/iwasm/interpreter/wasm_interp_fast.c
Original file line number Diff line number Diff line change
Expand Up @@ -1576,14 +1576,18 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module,
global = globals + global_idx;
global_addr = get_global_addr(global_data, global);
aux_stack_top = frame_lp[GET_OFFSET()];
if (aux_stack_top <= exec_env->aux_stack_boundary.boundary) {
wasm_set_exception(module, "wasm auxiliary stack overflow");
goto got_exception;
}
if (aux_stack_top > exec_env->aux_stack_bottom.bottom) {
wasm_set_exception(module,
"wasm auxiliary stack underflow");
goto got_exception;
if (wasm_exec_env_is_aux_stack_managed_by_runtime(exec_env)) {
if (aux_stack_top
<= exec_env->aux_stack_boundary.boundary) {
wasm_set_exception(module,
"wasm auxiliary stack overflow");
goto got_exception;
}
if (aux_stack_top > exec_env->aux_stack_bottom.bottom) {
wasm_set_exception(module,
"wasm auxiliary stack underflow");
goto got_exception;
}
}
*(int32 *)global_addr = aux_stack_top;
#if WASM_ENABLE_MEMORY_PROFILING != 0
Expand Down
5 changes: 3 additions & 2 deletions core/iwasm/libraries/lib-pthread/lib_pthread_wrapper.c
Original file line number Diff line number Diff line change
Expand Up @@ -619,8 +619,9 @@ pthread_create_wrapper(wasm_exec_env_t exec_env,
routine_args->module_inst = new_module_inst;

os_mutex_lock(&exec_env->wait_lock);
ret = wasm_cluster_create_thread(
exec_env, new_module_inst, pthread_start_routine, (void *)routine_args);
ret =
wasm_cluster_create_thread(exec_env, new_module_inst, true,
pthread_start_routine, (void *)routine_args);
if (ret != 0) {
os_mutex_unlock(&exec_env->wait_lock);
goto fail;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,8 @@ thread_spawn_wrapper(wasm_exec_env_t exec_env, uint32 start_arg)
thread_start_arg->start_func = start_func;

os_mutex_lock(&exec_env->wait_lock);
ret = wasm_cluster_create_thread(exec_env, new_module_inst, thread_start,
thread_start_arg);
ret = wasm_cluster_create_thread(exec_env, new_module_inst, false,
thread_start, thread_start_arg);
if (ret != 0) {
LOG_ERROR("Failed to spawn a new thread");
goto thread_spawn_fail;
Expand Down
29 changes: 18 additions & 11 deletions core/iwasm/libraries/thread-mgr/thread_manager.c
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,10 @@ free_aux_stack(WASMExecEnv *exec_env, uint32 start)
WASMModuleInstanceCommon *module_inst =
wasm_exec_env_get_module_inst(exec_env);

if (!wasm_exec_env_is_aux_stack_managed_by_runtime(exec_env)) {
return true;
}

bh_assert(start >= cluster->stack_size);

wasm_runtime_module_free(module_inst, start - cluster->stack_size);
Expand Down Expand Up @@ -534,7 +538,7 @@ thread_manager_start_routine(void *arg)

int32
wasm_cluster_create_thread(WASMExecEnv *exec_env,
wasm_module_inst_t module_inst,
wasm_module_inst_t module_inst, bool alloc_aux_stack,
void *(*thread_routine)(void *), void *arg)
{
WASMCluster *cluster;
Expand All @@ -550,16 +554,18 @@ wasm_cluster_create_thread(WASMExecEnv *exec_env,
if (!new_exec_env)
return -1;

if (!allocate_aux_stack(exec_env, &aux_stack_start, &aux_stack_size)) {
LOG_ERROR("thread manager error: "
"failed to allocate aux stack space for new thread");
goto fail1;
}
if (alloc_aux_stack) {
if (!allocate_aux_stack(exec_env, &aux_stack_start, &aux_stack_size)) {
LOG_ERROR("thread manager error: "
"failed to allocate aux stack space for new thread");
goto fail1;
}

/* Set aux stack for current thread */
if (!wasm_exec_env_set_aux_stack(new_exec_env, aux_stack_start,
aux_stack_size)) {
goto fail2;
/* Set aux stack for current thread */
if (!wasm_exec_env_set_aux_stack(new_exec_env, aux_stack_start,
aux_stack_size)) {
goto fail2;
}
}

if (!wasm_cluster_add_exec_env(cluster, new_exec_env))
Expand All @@ -581,7 +587,8 @@ wasm_cluster_create_thread(WASMExecEnv *exec_env,
wasm_cluster_del_exec_env(cluster, new_exec_env);
fail2:
/* free the allocated aux stack space */
free_aux_stack(exec_env, aux_stack_start);
if (alloc_aux_stack)
free_aux_stack(exec_env, aux_stack_start);
fail1:
wasm_exec_env_destroy(new_exec_env);
return -1;
Expand Down
2 changes: 1 addition & 1 deletion core/iwasm/libraries/thread-mgr/thread_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ wasm_exec_env_get_cluster(WASMExecEnv *exec_env);

int32
wasm_cluster_create_thread(WASMExecEnv *exec_env,
wasm_module_inst_t module_inst,
wasm_module_inst_t module_inst, bool alloc_aux_stack,
void *(*thread_routine)(void *), void *arg);

int32
Expand Down
7 changes: 4 additions & 3 deletions samples/wasi-threads/wasm-apps/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,13 @@ endif ()

set (CMAKE_SYSROOT "${WASI_SYSROOT}")
set (CMAKE_C_COMPILER "${WASI_SDK_DIR}/bin/clang")
set (CMAKE_ASM_COMPILER "${WASI_SDK_DIR}/bin/clang")
set (CMAKE_C_COMPILER_TARGET "wasm32-wasi")

function (compile_sample SOURCE_FILE)
get_filename_component (FILE_NAME ${SOURCE_FILE} NAME_WLE)
set (WASM_MODULE ${FILE_NAME}.wasm)
add_executable (${WASM_MODULE} ${SOURCE_FILE})
add_executable (${WASM_MODULE} ${SOURCE_FILE} ${ARGN})

target_compile_options (${WASM_MODULE} PRIVATE
-pthread -ftls-model=local-exec)
Expand All @@ -34,5 +35,5 @@ function (compile_sample SOURCE_FILE)
)
endfunction ()

compile_sample(no_pthread.c)
compile_sample(exception_propagation.c)
compile_sample(no_pthread.c wasi_thread_start.S)
compile_sample(exception_propagation.c wasi_thread_start.S)
36 changes: 26 additions & 10 deletions samples/wasi-threads/wasm-apps/exception_propagation.c
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,21 @@
#include <stdlib.h>
#include <stdio.h>
#include <assert.h>
#include <wasi/api.h>
#include <semaphore.h>
#include <stdbool.h>
#include <unistd.h>

#include "wasi_thread_start.h"

#define TIMEOUT_SECONDS 10
#define NUM_THREADS 3
static sem_t sem;

typedef struct {
start_args_t base;
bool throw_exception;
} shared_t;

void
run_long_task()
{
Expand All @@ -26,12 +32,12 @@ run_long_task()
sleep(1);
}

__attribute__((export_name("wasi_thread_start"))) void
wasi_thread_start(int thread_id, int *start_arg)
void
__wasi_thread_start_C(int thread_id, int *start_arg)
{
bool has_to_throw_exception = (bool)start_arg;
shared_t *data = (shared_t *)start_arg;

if (has_to_throw_exception) {
if (data->throw_exception) {
// Wait for all other threads (including main thread) to be ready
printf("Waiting before throwing exception\n");
for (int i = 0; i < NUM_THREADS; i++)
Expand All @@ -52,26 +58,36 @@ wasi_thread_start(int thread_id, int *start_arg)
int
main(int argc, char **argv)
{
int thread_id = -1;
int thread_id = -1, i;
shared_t data[NUM_THREADS] = { 0 };

if (sem_init(&sem, 0, 0) != 0) {
printf("Failed to init semaphore\n");
return EXIT_FAILURE;
}

for (i = 0; i < NUM_THREADS; i++) {
// No graceful memory free to simplify the example
if (!start_args_init(&data[i].base)) {
printf("Failed to allocate thread's stack\n");
return EXIT_FAILURE;
}
}

// Create a thread that throws an exception
thread_id = __wasi_thread_spawn((void *)true);
data[0].throw_exception = true;
thread_id = __wasi_thread_spawn(&data[0]);
if (thread_id < 0) {
printf("Failed to create thread: %d\n", thread_id);
return EXIT_FAILURE;
}

// Create two additional threads to test exception propagation
thread_id = __wasi_thread_spawn((void *)false);
thread_id = __wasi_thread_spawn(&data[1]);
if (thread_id < 0) {
printf("Failed to create thread: %d\n", thread_id);
return EXIT_FAILURE;
}
thread_id = __wasi_thread_spawn((void *)false);
thread_id = __wasi_thread_spawn(&data[2]);
if (thread_id < 0) {
printf("Failed to create thread: %d\n", thread_id);
return EXIT_FAILURE;
Expand Down
27 changes: 20 additions & 7 deletions samples/wasi-threads/wasm-apps/no_pthread.c
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,20 @@
#include <stdlib.h>
#include <stdio.h>
#include <assert.h>
#include <wasi/api.h>

#include "wasi_thread_start.h"
loganek marked this conversation as resolved.
Show resolved Hide resolved

static const int64_t SECOND = 1000 * 1000 * 1000;

typedef struct {
start_args_t base;
int th_ready;
int value;
int thread_id;
} shared_t;

__attribute__((export_name("wasi_thread_start"))) void
wasi_thread_start(int thread_id, int *start_arg)
void
__wasi_thread_start_C(int thread_id, int *start_arg)
{
shared_t *data = (shared_t *)start_arg;

Expand All @@ -38,24 +40,35 @@ wasi_thread_start(int thread_id, int *start_arg)
int
main(int argc, char **argv)
{
shared_t data = { 0, 52, -1 };
shared_t data = { { NULL }, 0, 52, -1 };
int thread_id;
int ret = EXIT_SUCCESS;

if (!start_args_init(&data.base)) {
printf("Stack allocation for thread failed\n");
return EXIT_FAILURE;
}

thread_id = __wasi_thread_spawn(&data);
if (thread_id < 0) {
printf("Failed to create thread: %d\n", thread_id);
return EXIT_FAILURE;
ret = EXIT_FAILURE;
goto final;
}

if (__builtin_wasm_memory_atomic_wait32(&data.th_ready, 0, SECOND) == 2) {
printf("Timeout\n");
return EXIT_FAILURE;
ret = EXIT_FAILURE;
goto final;
}

printf("Thread completed, new value: %d, thread id: %d\n", data.value,
data.thread_id);

assert(thread_id == data.thread_id);

return EXIT_SUCCESS;
final:
start_args_deinit(&data.base);

return ret;
}
22 changes: 22 additions & 0 deletions samples/wasi-threads/wasm-apps/wasi_thread_start.S
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# A slightly modified copy of the wasi-libc implementation
# https://github.com/WebAssembly/wasi-libc/pull/376/
.globaltype __stack_pointer, i32
.functype __wasi_thread_start_C (i32, i32) -> ()

.globl wasi_thread_start

wasi_thread_start:
loganek marked this conversation as resolved.
Show resolved Hide resolved
.functype wasi_thread_start (i32, i32) -> ()

# Set up the minimum C environment.
# Note: offsetof(start_arg, stack) == 0
local.get 1 # start_arg
i32.load 0 # stack
global.set __stack_pointer

# Make the C function do the rest of work.
local.get 0 # tid
local.get 1 # start_arg
call __wasi_thread_start_C

end_function
32 changes: 32 additions & 0 deletions samples/wasi-threads/wasm-apps/wasi_thread_start.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright (C) 2022 Amazon.com Inc. or its affiliates. All rights reserved.
* SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
*/
#ifndef WASI_THREAD_START_H
#define WASI_THREAD_START_H

#define STACK_SIZE 1024

typedef struct {
void *stack;
} start_args_t;

static inline int
start_args_init(start_args_t *start_args)
{
start_args->stack = malloc(STACK_SIZE);
if (!start_args->stack) {
return 0;
}

start_args->stack += STACK_SIZE;
return 1;
}

static inline void
start_args_deinit(start_args_t *start_args)
{
free(start_args->stack - STACK_SIZE);
}

#endif