-
Notifications
You must be signed in to change notification settings - Fork 72
v8: emit backtrace when trap happens. #66
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
Conversation
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.
Thanks, this looks great!
Regarding your questions:
- I'm confused as to why enabling this feature would expose Wasm's stack to the public?
- Yes, we should support this in WAVM as well.
- I'd like to defer merging this until it's available in Chromium's BETA channel (so ~6 weeks or so?).
Never mind, I just confused the meaning of "sandbox" in WASM. I edited the description |
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
590df70
to
5cb4a07
Compare
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
…t into trace-print
Signed-off-by: mathetake <takeshi@tetrate.io>
Now the trace looks like this:
|
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
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.
Thanks! LGTM sans typo.
Could you re-kick the CI via an empty commit? It looks that CLA bot got stuck. |
Signed-off-by: mathetake <takeshi@tetrate.io>
f2d9170
to
4f815f5
Compare
done |
if (start + size != pos) { | ||
// indicates the malformed function name subsection, so clear the stored indexes. | ||
function_names_index_ = {}; | ||
} |
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.
Nit: if this fails then we should probably break
from the loop, since we can't trust the pos
anyway... But I just realized that we don't do this type of verification when parsing bytecode in other places, so feel free to drop this whole if
block if you prefer.
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 trust given binaries anywhere while parsing so far 😄 maybe we should decide and stick to whether or not validate binary or not.
will add break for now.
Signed-off-by: mathetake <takeshi@tetrate.io>
…t into trace-print
src/v8/v8.cc
Outdated
if (*pos++ != 1) { | ||
pos += parseVarint(pos, end); | ||
} else { | ||
auto size = parseVarint(pos, end); |
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.
Actually, we need to verify this value, i.e.:
if (size == static_cast<uint32_t>(-1) || pos + size > end) {
function_names_index_ = {};
return;
}
Also, use const auto
to match rest of the code.
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
wait, something's broken |
Signed-off-by: mathetake <takeshi@tetrate.io>
now looks good 😄
|
This PR tries to implement the feature for emitting backtrace information when trap happens with V8 runtime. This would improve extension developers' debuggability dramatically.
summary
I fixed the bug in wasm-c-api implemented in V8 and the patch has been merged to the V8's upstream: https://chromium-review.googlesource.com/c/v8/v8/+/2465353. Now it's possible to extract backtrace information from V8 when
trap
happens when we link against the latest V8 commit.The example message is like this:
from the the following Go SDK's code:
The way I implemented is
Trap::trace()
API to get stacked function frames (which do not contain function names)name
custom section (ref. wasm-spec) to get actual function names combined with frame informations acquired in (1).message design
I am not sure how the backtrace message should look like, so I would like to have opinions as much as possible.
As of now, I just followed the style of wasmtime where
<unknown>
is themodule
name subsection in thename
custom section:Future works
This would not work until link the host against the latest upstream of V8