Copy callstack API #4033
Copy callstack API #4033
Conversation
g0djan
requested review from
loganek,
lum1n0us,
no1wudi,
TianlongLiang,
wenyongh,
xujuntwt95329 and
yamt
as code owners
|
@loganek addressed all your comments and rebased to fix the checks. The last failing check is CI issue and there's another PR that will fix it. |
|
@lum1n0us could you take a look at this PR? |
|
@yamt could you please take a look at this PR? |
| * interruption from another thread if next variables hold valid pointers | ||
| * - exec_env | ||
| * - exec_env->module_inst | ||
| * - exec_env->module_inst->module |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@yamt @TianlongLiang @wenyongh Hey guys could someone reply here or provide further review please? Sorry for pinging multiple times
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
|
Should be possible yeah, I can try
It's possible to bring this logic to WAMR, but if we are to do this I would like to have wasm_iterate_callstack API available first and then open a separate PR for dumping callstack on signal interruption
This doesn't go well with async-signal-safety, that's why it's not part of it. But it seems you'd like to have a general approach to inspect stack in a safe manner rather than thread-safety itself |
I started refactoring that, So possible change comes down to replacing this loop for wasm_interp But it doesn't make code simpler imo, it just adds more boilerplate internally. Do you see it useful/meaningful to do so @lum1n0us ? |
|
@lum1n0us I addressed security concerns and made API read-only by reducing it to a copy API instead of using user defined callback. Also I hid the feature behind a feature flag to avoid accidental use of a non thread safe API Could you please take a look at it? |
core/iwasm/aot/aot_runtime.c
Outdated
| bool is_top_index_in_range = | ||
| top_boundary >= top && top >= (bottom + sizeof(AOTTinyFrame)); | ||
| if (!is_top_index_in_range) { | ||
| return count; |
There was a problem hiding this comment.
Definitely a critical error; please make sure to log it.
core/iwasm/aot/aot_runtime.c
Outdated
| bool is_top_aligned_with_bottom = | ||
| (unsigned long)(top - bottom) % sizeof(AOTTinyFrame) == 0; | ||
| if (!is_top_aligned_with_bottom) { | ||
| return count; |
There was a problem hiding this comment.
Definitely a critical error; please make sure to log it.
| if (!cur_frame->function) { | ||
| cur_frame = cur_frame->prev_frame; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I observed that the PR only skips this case in interpreter mode. Is this intentional?
addressed all |
New WAMR public API to copy runtime call stack frames.
CAUTION: this APIs is not thread safe, that's why it's hidden behind feature flag for now. If you need to call it from another thread ensure the passed exec_env is suspended.
Our use case
Sometimes WAMR runtime gets stuck in production and we have no data where in the code compiled to WASM it happens. We currently only track such situations in a separate native thread. To increase visibility into the problem we developed internal solution that requires presence of this API in WAMR. If a separate thread finds that the WASM VM thread has stuck, it interrupts it with a user defined signal and calls this API to collect callstack. The main complexity is maintaining async-signal-safety and avoiding segfaults. For that we're maintaining atomic copies of
exec_env,exec_env->module_inst,exec_env->module_inst->module. Those copies are always set to NULL before the referenced memory is freed. Before a call to this API those copies are always checked for validity. In our use case scenario we guarantee ourselves only absence of crashes but we realize that the frame data that we collect might be invalidated due to a signal interruption. However it's highly unlikely and is not a concern for us.Have we tried existing WAMR APIs for our usecase?
Yes, we've tried suggested by maintainers
wasm_cluster_suspend_threadandwasm_runtime_terminate.wasm_cluster_suspend_threaddoesn't suit us either. Even if it did we'd still need API to iterate over stackframes.