-
Notifications
You must be signed in to change notification settings - Fork 111
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
Implement read/write of DW_OP_WASM_location #546
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 @philipc! Code itself looks good to me.
However, I have a question: is the intention never to support evaluating these Wasm operations?
If so, I disagree with that decision. I think this support will be necessary to implement for Wasm hosts that want to expose sandboxed, user-level debugging of Wasm code without also exposing the native host code's machine state. That is, in situations where the "Wasm DWARF" isn't translated to "native DWARF" alongside the Wasm's translation into native code, for example in web browsers.
If the intention is to support evaluating these Wasm ops eventually, then it may make sense to just implement that support now (I can do it in a follow up PR, unless you want to do it) because the UnsupportedEvaluation
error will become unused/go away as soon as this support is implemented, and it would be better not to have multiple breaking changes to Error
in back-to-back releases.
/// Represents `DW_OP_WASM_location 0x01`. | ||
WasmGlobal(u32), |
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.
Will this always be DW_OP_WASM_location 0x01
, or can it also represent DW_OP_WASM_location 0x03
? Fine if the former, just curious.
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.
Currently always 0x01. I don't know the history on why 0x03 was added. My guess is that it is for cases where you don't know the index yet, so you need to be sure there is enough space to write the index later. If so, that will need to use a different variant that somehow records the location of the index so that you know where to write it later.
No. My primary reason is that I don't understand how these operations should be evaluated (see comments in #544).
That's be great! |
Does not support evaluation yet though.
Does not support evaluation though.
Sample dwarfdump output:
Fixes #544