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

Instruction breakpoints: What is the offset ? #341

Closed
puremourning opened this issue Oct 11, 2022 · 13 comments · Fixed by #436
Closed

Instruction breakpoints: What is the offset ? #341

puremourning opened this issue Oct 11, 2022 · 13 comments · Fixed by #436
Assignees
Labels
clarification Protocol clarification

Comments

@puremourning
Copy link
Contributor

In the setInstructionBreakpoints request, we have this:

  /**
   * The offset from the instruction reference.
   * This can be negative.
   */
  offset?: number;

Is this an instruction offset (like in the disassemble request), or a memory offset (in bytes) like in the readMemory request?

Can we specify it?

@connor4312 connor4312 added the clarification Protocol clarification label Oct 11, 2022
@connor4312
Copy link
Member

Yes, we should clarify and say "The offset in bytes from..."

@puremourning
Copy link
Contributor Author

So, this requires the client to parse and understand the 'address' field of the returned instructions from a disassemble request?

I guess that's fine, but it does put more constraints on what the "implementation defined" invalid instruction DisassembledInstruction contains. I suggest we state that an address of 0x0 is required for invalid instructions.

@gregg-miskelly
Copy link
Member

I am not following why it constrains the implementation for an invalid instruction, but I do think it points out one flaw in the spec:

"instructionBytes": {
					"type": "string",
					"description": "Raw bytes representing the instruction and its operands, in an implementation-defined format."
				},

I don't think this should have been listed as implementation defined. I think we need to define the set of allowed formats for this so that one can calculate the size of a DisassembledInstruction.

CC @andrewcrawley in case I missed something.

@gregg-miskelly
Copy link
Member

gregg-miskelly commented Oct 11, 2022

Oh, duh, I should have read this more closely, yes, the address field is designed to be understandable by the client. So that is how offsets can be calculated.

@puremourning
Copy link
Contributor Author

Oh, duh, I should have read this more closely, yes, the address field is designed to be understandable by the client. So that is how offsets can be calculated.

Right, that's fine - the spec states that 0x... means hex and anything else is decimal. Cool cool. It's just a little more complicated by the requirement (that's absolutely required for the current API to work) that the disassembly request pads with "implementation defined" invalid instructions. In practice, it's not going to work if a user tries to put a breakpoint on an "invalid" instruction, but it shouldn't lead to badness.

@gregg-miskelly
Copy link
Member

Can you explain why that would be the case that it would "lead to badness"?

@puremourning
Copy link
Contributor Author

I just thought it might if the adapter put random number in the address for the invalid instruction. Then the client would try to calculate the offset and end up with a(nother) random number and ask the debugger to put an instruction breakpoint on a random number. There's nothing specific that would be badness here, but it might be better if client is told "0x0 means invalid address" and therefore can just tell the user "That's not a real instruction, you can't put a breakpoint there". It's not a big deal.

@gregg-miskelly
Copy link
Member

Got it.

For programming environments with a flat address space (ex: x86/x64/ARM native debuggers) where you can pretend that the memory space is infinite (ex: subtracting 1 from address 0 just gets you back to 0xffffffffffffffff), there should really only be two reasons for an invalid instruction:

  1. Instruction is invalid because it isn't part of a known instruction sequence
  2. Instruction is invalid because the memory isn't readable

In either case, all instructions have a valid numeric instruction address.

For programming environments where the address space isn't so simple: I could potentially see value in some sort of reserved 'invalid value' for address. Maybe just empty string?

@rohit-kumar-j

This comment was marked as off-topic.

@puremourning

This comment was marked as off-topic.

@connor4312
Copy link
Member

connor4312 commented Sep 11, 2023

Hi there. Circling back here, how about these two changes?

The aforementioned clarification:

interface InstructionBreakpoint {
  /**
   * The offset from the instruction reference in bytes.
   * This can be negative.
   */
  offset?: number;

Unfortunately allowing the adapter to return less than the requested number of instructions is tricky to do in a backwards-compatible way, but an additional presentation hint for the DisassembledInstruction may help:

interface DisassembledInstruction {
	/**
	 * A hint for how to present the source in the UI.
	 * A value of `invalid` can be used to indicate this instruction is 'filler'
	 * and cannot be reached by the program. For example, unreadable memory
	 * addresses may be presented is 'invalid.'
	 */
	presentationHint?: 'normal' | 'invalid';

@connor4312 connor4312 added this to the September 2023 milestone Sep 25, 2023
@roblourens
Copy link
Member

Unfortunately allowing the adapter to return less than the requested number of instructions is tricky to do in a backwards-compatible way,

I wonder if we could allow that using a client-side supports flag? But that's a bit different than what other feature flags are controlling, maybe it's a bit weird. Otherwise, the presentationHint seems fine

@puremourning
Copy link
Contributor Author

Agree. Both changes seem practical to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Protocol clarification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants