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

Copy callstack API #4033

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
68e4534
Iterate callstack API
Jan 17, 2025
d0c6da1
wamr bool type
Jan 27, 2025
1f4d3dd
clang-format
Jan 27, 2025
1b82ccc
meaning of the return bool type in the callback
Jan 27, 2025
813831d
keep devs notes out of public API
Jan 27, 2025
bf6b155
format
Jan 27, 2025
c8b8731
support standard frames as well
Jan 27, 2025
9ff8052
format
Jan 27, 2025
6bfc088
Calculate func_index instead of adding an extra field to wasm frame
Jan 28, 2025
b6daacb
ignore frames with no function
Jan 28, 2025
5bfbfd5
update typo in the comment
Jan 28, 2025
478b373
update signature
Jan 28, 2025
b9039f9
Merge branch 'main' into godjan/iterate_callstack
Jan 28, 2025
fb6c05e
add correct frame size for aot standard frames
Jan 28, 2025
f7204bd
standard frame is not supported when GC is enabled
Jan 28, 2025
267379c
Merge branch 'main' into godjan/iterate_callstack
Feb 5, 2025
32338bb
Copy read only API behind a flag instead of using user defined callback
Feb 24, 2025
cc3f0a0
Cleaning up
Feb 24, 2025
188eb1c
remove unnecessary includes
Feb 25, 2025
857e6b7
formatting
Feb 26, 2025
a5d8c0b
define if not defined
Feb 26, 2025
99cb6ec
formatting
Feb 26, 2025
fc3077b
address comments
Feb 27, 2025
bda012e
formatting
Feb 27, 2025
0b5084c
remove spare diff line
Feb 27, 2025
bc00b3e
address comments
Mar 3, 2025
ffcc157
clang format
Mar 3, 2025
32d0f55
spare line
Mar 3, 2025
6166788
spare lines
Mar 3, 2025
56bb7e7
last fixes
Mar 5, 2025
811f35b
identation
Mar 5, 2025
e488345
fix bug for return value when skip_n is passed
Mar 5, 2025
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
5 changes: 5 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ if (NOT DEFINED WAMR_BUILD_LIB_WASI_THREADS)
set (WAMR_BUILD_LIB_WASI_THREADS 0)
endif ()

if (NOT DEFINED WAMR_ENABLE_COPY_CALLSTACK)
# Disable copy callstack by default
set (WAMR_ENABLE_COPY_CALLSTACK 0)
endif()

if (NOT DEFINED WAMR_BUILD_MINI_LOADER)
# Disable wasm mini loader by default
set (WAMR_BUILD_MINI_LOADER 0)
Expand Down
8 changes: 8 additions & 0 deletions build-scripts/config_common.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,14 @@ if (WAMR_BUILD_SHARED_HEAP EQUAL 1)
message (" Shared heap enabled")
endif()

if (WAMR_ENABLE_COPY_CALLSTACK EQUAL 1)
add_definitions (-DWAMR_ENABLE_COPY_CALLSTACK=1)
message(" Copy callstack enabled")
else ()
add_definitions (-DWAMR_ENABLE_COPY_CALLSTACK=0)
message(" Copy callstack disabled")
endif()

if (WAMR_BUILD_MEMORY64 EQUAL 1)
# if native is 32-bit or cross-compiled to 32-bit
if (NOT WAMR_BUILD_TARGET MATCHES ".*64.*")
Expand Down
4 changes: 4 additions & 0 deletions core/config.h
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,10 @@
#error "Heap aux stack allocation must be enabled for WASI threads"
#endif

#ifndef WAMR_ENABLE_COPY_CALLSTACK
#define WAMR_ENABLE_COPY_CALLSTACK 0
#endif

#ifndef WASM_ENABLE_BASE_LIB
#define WASM_ENABLE_BASE_LIB 0
#endif
Expand Down
129 changes: 129 additions & 0 deletions core/iwasm/aot/aot_runtime.c
Original file line number Diff line number Diff line change
Expand Up @@ -4103,6 +4103,135 @@ aot_frame_update_profile_info(WASMExecEnv *exec_env, bool alloc_frame)
}
#endif /* end of WASM_ENABLE_AOT_STACK_FRAME != 0 */

#if WAMR_ENABLE_COPY_CALLSTACK != 0
uint32
aot_copy_callstack_tiny_frame(WASMExecEnv *exec_env, wasm_frame_t *buffer,
const uint32 length, const uint32 skip_n,
char *error_buf, uint32 error_buf_size)
{
/*
* Note for devs: please refrain from such modifications inside of
* aot_copy_callstack_tiny_frame
* - any allocations/freeing memory
* - dereferencing any pointers other than: exec_env, exec_env->module_inst,
* exec_env->module_inst->module, pointers between stack's bottom and
* top_boundary For more details check wasm_copy_callstack in
* wasm_export.h
*/
uint8 *top_boundary = exec_env->wasm_stack.top_boundary;
uint8 *top = exec_env->wasm_stack.top;
uint8 *bottom = exec_env->wasm_stack.bottom;
uint32 count = 0;

bool is_top_index_in_range =
top_boundary >= top && top >= (bottom + sizeof(AOTTinyFrame));
if (!is_top_index_in_range) {
char *err_msg =
"Top of the stack pointer is outside of the stack boundaries";
strncpy(error_buf, err_msg, error_buf_size);
return 0;
}
bool is_top_aligned_with_bottom =
(unsigned long)(top - bottom) % sizeof(AOTTinyFrame) == 0;
if (!is_top_aligned_with_bottom) {
char *err_msg = "Top of the stack is not aligned with the bottom";
strncpy(error_buf, err_msg, error_buf_size);
return 0;
}

AOTTinyFrame *frame = (AOTTinyFrame *)(top - sizeof(AOTTinyFrame));
WASMCApiFrame record_frame;
while (frame && (uint8_t *)frame >= bottom && count < (skip_n + length)) {
if (count < skip_n) {
++count;
frame -= 1;
continue;
}
record_frame.instance = exec_env->module_inst;
record_frame.module_offset = 0;
record_frame.func_index = frame->func_index;
record_frame.func_offset = frame->ip_offset;
buffer[count - skip_n] = record_frame;
frame -= 1;
++count;
}
return count;
}

uint32
aot_copy_callstack_standard_frame(WASMExecEnv *exec_env, wasm_frame_t *buffer,
const uint32 length, const uint32 skip_n,
char *error_buf, uint32_t error_buf_size)
{
/*
* Note for devs: please refrain from such modifications inside of
* aot_iterate_callstack_standard_frame
* - any allocations/freeing memory
* - dereferencing any pointers other than: exec_env, exec_env->module_inst,
* exec_env->module_inst->module, pointers between stack's bottom and
* top_boundary For more details check wasm_iterate_callstack in
* wasm_export.h
*/
#if WASM_ENABLE_GC == 0
WASMModuleInstance *module_inst =
(WASMModuleInstance *)wasm_exec_env_get_module_inst(exec_env);
AOTFrame *cur_frame = (AOTFrame *)wasm_exec_env_get_cur_frame(exec_env);
uint8 *top_boundary = exec_env->wasm_stack.top_boundary;
uint8 *bottom = exec_env->wasm_stack.bottom;
uint32 frame_size = (uint32)offsetof(AOTFrame, lp);
uint32 count = 0;

WASMCApiFrame record_frame;
while (cur_frame && (uint8_t *)cur_frame >= bottom
&& (uint8_t *)cur_frame + frame_size <= top_boundary
&& count < (skip_n + length)) {
if (count < skip_n) {
++count;
cur_frame = cur_frame->prev_frame;
continue;
}
record_frame.instance = module_inst;
record_frame.module_offset = 0;
record_frame.func_index = (uint32)cur_frame->func_index;
record_frame.func_offset = (uint32)cur_frame->ip_offset;
buffer[count - skip_n] = record_frame;
cur_frame = cur_frame->prev_frame;
++count;
}
return count;
#else
/*
* TODO: add support for standard frames when GC is enabled
* now it poses a risk due to variable size of the frame
*/
#endif
}

uint32
aot_copy_callstack(WASMExecEnv *exec_env, wasm_frame_t *buffer,
const uint32 length, const uint32 skip_n, char *error_buf,
uint32_t error_buf_size)
{
/*
* Note for devs: please refrain from such modifications inside of
* aot_iterate_callstack
* - any allocations/freeing memory
* - dereferencing any pointers other than: exec_env, exec_env->module_inst,
* exec_env->module_inst->module, pointers between stack's bottom and
* top_boundary For more details check wasm_iterate_callstack in
* wasm_export.h
*/
if (!is_tiny_frame(exec_env)) {
return aot_copy_callstack_standard_frame(
exec_env, buffer, length, skip_n, error_buf, error_buf_size);
}
else {
return aot_copy_callstack_tiny_frame(exec_env, buffer, length, skip_n,
error_buf, error_buf_size);
}
}
#endif // WAMR_ENABLE_COPY_CALLSTACK

#if WASM_ENABLE_DUMP_CALL_STACK != 0
bool
aot_create_call_stack(struct WASMExecEnv *exec_env)
Expand Down
7 changes: 7 additions & 0 deletions core/iwasm/aot/aot_runtime.h
Original file line number Diff line number Diff line change
Expand Up @@ -777,6 +777,13 @@ aot_frame_update_profile_info(WASMExecEnv *exec_env, bool alloc_frame);
bool
aot_create_call_stack(struct WASMExecEnv *exec_env);

#if WAMR_ENABLE_COPY_CALLSTACK != 0
uint32
aot_copy_callstack(WASMExecEnv *exec_env, wasm_frame_t *buffer,
const uint32 length, const uint32 skip_n, char *error_buf,
uint32_t error_buf_size);
#endif // WAMR_ENABLE_COPY_CALLSTACK

/**
* @brief Dump wasm call stack or get the size
*
Expand Down
39 changes: 39 additions & 0 deletions core/iwasm/common/wasm_runtime_common.c
Original file line number Diff line number Diff line change
Expand Up @@ -1740,6 +1740,45 @@ wasm_runtime_destroy_exec_env(WASMExecEnv *exec_env)
wasm_exec_env_destroy(exec_env);
}

#if WAMR_ENABLE_COPY_CALLSTACK != 0
uint32
wasm_copy_callstack(const wasm_exec_env_t exec_env, wasm_frame_t *buffer,
const uint32 length, const uint32 skip_n, char *error_buf,
uint32_t error_buf_size)
{
/*
* Note for devs: please refrain from such modifications inside of
* wasm_copy_callstack to preserve async-signal-safety
* - any allocations/freeing memory
* - dereferencing any pointers other than: exec_env, exec_env->module_inst,
* exec_env->module_inst->module, pointers between stack's bottom and
* top_boundary For more details check wasm_copy_callstack in
* wasm_export.h
*/
#if WASM_ENABLE_DUMP_CALL_STACK
WASMModuleInstance *module_inst =
(WASMModuleInstance *)get_module_inst(exec_env);

#if WASM_ENABLE_INTERP != 0
if (module_inst->module_type == Wasm_Module_Bytecode) {
return wasm_interp_copy_callstack(exec_env, buffer, length, skip_n,
error_buf, error_buf_size);
}
#endif

#if WASM_ENABLE_AOT != 0
if (module_inst->module_type == Wasm_Module_AoT) {
return aot_copy_callstack(exec_env, buffer, length, skip_n, error_buf,
error_buf_size);
}
#endif
#endif
char *err_msg = "No copy_callstack API was actually executed";
strncpy(error_buf, err_msg, error_buf_size);
return 0;
}
#endif // WAMR_ENABLE_COPY_CALLSTACK

bool
wasm_runtime_init_thread_env(void)
{
Expand Down
20 changes: 7 additions & 13 deletions core/iwasm/common/wasm_runtime_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -464,19 +464,6 @@ typedef struct WASMRegisteredModule {
typedef package_type_t PackageType;
typedef wasm_section_t WASMSection, AOTSection;

typedef struct wasm_frame_t {
/* wasm_instance_t */
void *instance;
uint32 module_offset;
uint32 func_index;
uint32 func_offset;
const char *func_name_wp;

uint32 *sp;
uint8 *frame_ref;
uint32 *lp;
} WASMCApiFrame;

#if WASM_ENABLE_JIT != 0
typedef struct LLVMJITOptions {
uint32 opt_level;
Expand Down Expand Up @@ -652,6 +639,13 @@ wasm_runtime_create_exec_env(WASMModuleInstanceCommon *module_inst,
WASM_RUNTIME_API_EXTERN void
wasm_runtime_destroy_exec_env(WASMExecEnv *exec_env);

#if WAMR_ENABLE_COPY_CALLSTACK != 0
WASM_RUNTIME_API_EXTERN uint32_t
wasm_copy_callstack(const wasm_exec_env_t exec_env, wasm_frame_t *buffer,
const uint32 length, const uint32 skip_n, char *error_buf,
uint32 error_buf_size);
#endif // WAMR_ENABLE_COPY_CALLSTACK

/* See wasm_export.h for description */
WASM_RUNTIME_API_EXTERN WASMModuleInstanceCommon *
wasm_runtime_get_module_inst(WASMExecEnv *exec_env);
Expand Down
44 changes: 44 additions & 0 deletions core/iwasm/include/wasm_export.h
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,21 @@ typedef WASMFunctionInstanceCommon *wasm_function_inst_t;
struct WASMMemoryInstance;
typedef struct WASMMemoryInstance *wasm_memory_inst_t;

typedef struct wasm_frame_t {
/* wasm_instance_t */
void *instance;
uint32_t module_offset;
uint32_t func_index;
uint32_t func_offset;
const char *func_name_wp;

uint32_t *sp;
uint8_t *frame_ref;
uint32_t *lp;
} WASMCApiFrame;

typedef WASMCApiFrame wasm_frame_t;

/* WASM section */
typedef struct wasm_section_t {
struct wasm_section_t *next;
Expand Down Expand Up @@ -864,6 +879,35 @@ wasm_runtime_create_exec_env(wasm_module_inst_t module_inst,
WASM_RUNTIME_API_EXTERN void
wasm_runtime_destroy_exec_env(wasm_exec_env_t exec_env);

/**
* @brief Copy callstack frames.
*
* Caution: This is not a thread-safe function. Ensure the exec_env
* is suspended before calling it from another thread.
*
* Usage: In the callback to read frames fields use APIs
* for wasm_frame_t from wasm_c_api.h
*
* Note: The function is async-signal-safe if called with verified arguments.
* Meaning it's safe to call it from a signal handler even on a signal
* interruption from another thread if next variables hold valid pointers
* - exec_env
* - exec_env->module_inst
* - exec_env->module_inst->module
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO, making this complex functionality async-signal-safe is too much maintenance burden, especially when wamr itself doesn't rely on the property at all.

given that you need to suspend the target thread anyway, why don't you call this from another thread?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

async-signal-safe is too much maintenance burden

Yeah, we understand that and I kept it simple as much as possible. Basically non async-signal-safe implementation would be different only in a few checks removed and there wouldn't be comments that I added in the code.

Particularly this comment about validity of pointers is a theoretical problem atm. We don't know any platform yet where updating pointer variable might be interrupted by a signal, possibly after launch we will see that it never happens.

given that you need to suspend the target thread anyway, why don't you call this from another thread?

Do you mean using wasm_cluster_suspend_thread?
I tried that and there're 2 problems for us:

  • Now there’s no awaiting till thread actually gets suspended
  • Suspension happens only after certain checks so we're not getting stacktraces that we need. E.g. if there's sleep somewhere, there won't be sleep in stacktrace reported, it'd report calls after sleep has finished. If there's a deadlock stacktrace won't be reported at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@g0djan g0djan Feb 13, 2025

Choose a reason for hiding this comment

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

@yamt @TianlongLiang @wenyongh Hey guys could someone reply here or provide further review please? Sorry for pinging multiple times

Copy link
Collaborator

Choose a reason for hiding this comment

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

async-signal-safe is too much maintenance burden

Yeah, we understand that and I kept it simple as much as possible. Basically non async-signal-safe implementation would be different only in a few checks removed and there wouldn't be comments that I added in the code.

Particularly this comment about validity of pointers is a theoretical problem atm. We don't know any platform yet where updating pointer variable might be interrupted by a signal, possibly after launch we will see that it never happens.

given that you need to suspend the target thread anyway, why don't you call this from another thread?

Do you mean using wasm_cluster_suspend_thread?

i was not thinking about a specific way to suspend a thread.
this api is not expected to work on a running thread, is it?

I tried that and there're 2 problems for us:

* Now there’s no awaiting till thread actually gets suspended

* Suspension happens only after certain checks so we're not getting stacktraces that we need. E.g. if there's sleep somewhere, there won't be sleep in stacktrace reported, it'd report calls after sleep has finished. If there's a deadlock stacktrace won't be reported at all

wrt how to suspend a thread, i thought you were planning to use a signal to interrupt blocking system calls, right?
to avoid the signal affecting the target application behavior, depending on the kind of system calls, you might need to somehow wrap the system calls.
you might want to adapt the existing blocking-syscall wrapper api: https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/common/wasm_blocking_op.c
after all, i suspect we will end up with something which shares the guts of the underlying machinery with wasm_runtime_terminate.

or, maybe things like ptrace can be more flexible for your purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this api is not expected to work on a running thread, is it?

It's not. But I don't see that much benefit in calling this API from another thread compared to signal handler. Ensuring validity of pointers that are about to be dereferenced is the main restriction and it remains due to asynchronous nature of signal delivery

Copy link
Contributor Author

@g0djan g0djan Feb 14, 2025

Choose a reason for hiding this comment

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

depending on the kind of system calls, you might need to somehow wrap the system calls

is there an example that I could check out?

*
* @param exec_env the execution environment that containes frames
* @param buffer the buffer of size equal length * sizeof(wasm_frame_t) to copy
* frames to
* @param length the number of frames to copy
* @param skip_n the number of frames to skip from the top of the stack
*
* @return number of copied frames
*/
WASM_RUNTIME_API_EXTERN uint32_t
wasm_copy_callstack(const wasm_exec_env_t exec_env, wasm_frame_t *buffer,
const uint32_t length, const uint32_t skip_n,
char *error_buf, uint32_t error_buf_size);

/**
* Get the singleton execution environment for the instance.
*
Expand Down
51 changes: 51 additions & 0 deletions core/iwasm/interpreter/wasm_runtime.c
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

#include "wasm_runtime.h"
#include "wasm.h"
#include "wasm_c_api.h"
#include "wasm_exec_env.h"
#include "wasm_loader.h"
#include "wasm_interp.h"
#include "bh_common.h"
Expand Down Expand Up @@ -4195,6 +4197,55 @@ wasm_get_module_inst_mem_consumption(const WASMModuleInstance *module_inst,
#endif /* end of (WASM_ENABLE_MEMORY_PROFILING != 0) \
|| (WASM_ENABLE_MEMORY_TRACING != 0) */

#if WAMR_ENABLE_COPY_CALLSTACK != 0
uint32
wasm_interp_copy_callstack(WASMExecEnv *exec_env, wasm_frame_t *buffer,
uint32 length, uint32 skip_n, char *error_buf,
uint32_t error_buf_size)
{
/*
* Note for devs: please refrain from such modifications inside of
* wasm_interp_copy_callstack
* - any allocations/freeing memory
* - dereferencing any pointers other than: exec_env, exec_env->module_inst,
* exec_env->module_inst->module, pointers between stack's bottom and
* top_boundary For more details check wasm_copy_callstack in
* wasm_export.h
*/
WASMModuleInstance *module_inst =
(WASMModuleInstance *)wasm_exec_env_get_module_inst(exec_env);
WASMInterpFrame *cur_frame = wasm_exec_env_get_cur_frame(exec_env);
uint8 *top_boundary = exec_env->wasm_stack.top_boundary;
uint8 *bottom = exec_env->wasm_stack.bottom;
uint32 count = 0;

WASMCApiFrame record_frame;
while (cur_frame && (uint8_t *)cur_frame >= bottom
&& (uint8_t *)cur_frame + sizeof(WASMInterpFrame) <= top_boundary
&& count < (skip_n + length)) {
if (!cur_frame->function) {
cur_frame = cur_frame->prev_frame;
continue;
}
if (count < skip_n) {
++count;
cur_frame = cur_frame->prev_frame;
continue;
}
record_frame.instance = module_inst;
record_frame.module_offset = 0;
// It's safe to dereference module_inst->e because "e" is asigned only
// once in wasm_instantiate
record_frame.func_index =
(uint32)(cur_frame->function - module_inst->e->functions);
buffer[count - skip_n] = record_frame;
cur_frame = cur_frame->prev_frame;
++count;
}
return count;
}
#endif // WAMR_ENABLE_COPY_CALLSTACK

#if WASM_ENABLE_DUMP_CALL_STACK != 0
bool
wasm_interp_create_call_stack(struct WASMExecEnv *exec_env)
Expand Down
Loading
Loading