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

Implement read/write of DW_OP_WASM_location #546

Merged
merged 1 commit into from
Jan 29, 2021

Conversation

philipc
Copy link
Collaborator

@philipc philipc commented Jan 27, 2021

Does not support evaluation though.

Sample dwarfdump output:

DW_AT_frame_base            len 0x0007: ed03000000009f: DW_OP_WASM_location 0x3 0x0 DW_OP_stack_value

Fixes #544

Copy link
Member

@fitzgen fitzgen left a 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.

src/constants.rs Outdated Show resolved Hide resolved
src/read/endian_reader.rs Outdated Show resolved Hide resolved
src/read/op.rs Outdated Show resolved Hide resolved
Comment on lines +496 to +497
/// Represents `DW_OP_WASM_location 0x01`.
WasmGlobal(u32),
Copy link
Member

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.

Copy link
Collaborator Author

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.

@philipc
Copy link
Collaborator Author

philipc commented Jan 27, 2021

However, I have a question: is the intention never to support evaluating these Wasm operations?

No. My primary reason is that I don't understand how these operations should be evaluated (see comments in #544).

I can do it in a follow up PR

That's be great!

Does not support evaluation yet though.
@philipc philipc merged commit aa03ab8 into gimli-rs:master Jan 29, 2021
@philipc philipc deleted the wasm_loc branch January 29, 2021 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DW_OP_WASM_location is unsupported
2 participants