Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: main
Are you sure you want to change the base?
Copy callstack API #4033
Changes from 16 commits
68e4534
d0c6da1
1f4d3dd
1b82ccc
813831d
bf6b155
c8b8731
9ff8052
6bfc088
b6daacb
5bfbfd5
478b373
b9039f9
fb6c05e
f7204bd
267379c
32338bb
cc3f0a0
188eb1c
857e6b7
a5d8c0b
99cb6ec
fc3077b
bda012e
0b5084c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Do you mean using
wasm_cluster_suspend_thread
?I tried that and there're 2 problems for us:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yamt @lum1n0us
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there an example that I could check out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I observed that the PR only skips this case in interpreter mode. Is this intentional?