Proposal: remove requirement for exact instruction count for disassemble response

[suzmue]

The spec requires debug adapters to return exactly the number of instructions requested.

  /**
   * Number of instructions to disassemble starting at the specified location
   * and offset.
   * An adapter must return exactly this number of instructions - any
   * unavailable instructions should be replaced with an implementation-defined
   * 'invalid instruction' value.
   */
  instructionCount: number;

This presents an issue when there are not enough instructions available. The DA is then required to return an "implementation-defined" invalid instruction. It is unclear how those instructions will be used, and why they are even necessary to return. Scrolling through invalid instructions does not provide any additional information to users.

Also what the addresses should be for the invalid instructions is not defined in the spec so it is unclear how the editor can interpret them. For example, the VS Code implementation of a disassembly view will attempt to do a binary search using the addresses, which requires that the invalid instructions obey certain rules to implement this. This may also result in having to return negative addresses which also does not really make sense for an instruction.

Removing the requirement to return exactly the number of requested instructions would better define the expected disassembled instructions for both the client and server.

[puremourning]

Sorry to sort of hijack on this request, but if we're revisiting some of the disassembly stuff, I'd like to make a request: remove the requirement to supply instructionCount at all. here's what I'm thinking...

I'm looking to implement disassembly in vimspector but I'm looking at a lot of UI work due to the requirement to specify a number of instructions to disassemble (or, I have to ask the user to specify how many instructions, but ... how would they know?). Given that the instructionPointerReference comes from a StackFrame and that StackFrame probably knows the last instruction in the function (it can optionally specify the last line at least currently), could we have an optional adapter-supported disassembly where we can request the entire function related to the reference be disassembled and returned? This would make it trivial to display this in the editor without having to guess a number of instructions and/or update them in response to user actions.

An alternative would be to have a numberOfInstructions in the StackFrame, which would achieve the same result. Perhaps this is more straightforward, but requires the adapter to calculate it upfront even if not required.

Will need adapter authors' buy-in for it of course. I'm making an assumption that debuggers can provide this automatic "last" instruction for a function. It seems plausible to me.

Let me know if I should open a new issue for that rather than hijacking this one.

[yannickowow]

Hello,
Here are my points to this question, because I don't clearly see what is wrong with the current implementation.
Since the disassembly request is done through the UI, the instructionCount is driven by the amount of space available in the window. Thus, the request should clearly contain an amount of instruction to fetch. Without this, the client should multiply the number of request by the number of instruction to fill in the view.

Another way, if you don't want these kind of numbers, is to fetch disassembly lines by a greater amount of chunks (maybe fixed). But, if you take GDB as an example, it may contains fragmented ranges of disassembly (example: 0x80000 - 0x400000 and 0x600000 - 0x800000). With your implementation, I won't be able to scroll from 0x80000 to 0x600000 since I will face a dead-range. With the current implementation, I will see placeholders.

On my side, I would say that the instructionCount is clearly needed.

[haneefdm]

I think instructionCount is fine but the requirement that we return the exact number is what gave me headaches at the boundaries of memory regions. As to what addresses I used for the fake/dummy instructions, I just incremented (or decremented depending on direction) by the smallest instruction size/alignment valid for the architecture -- but I could have done anything. Also, I make it clear that these are fake instructions with the other properties. Not sure if something like that needs to be formalized, but if you do, that is fine too.

As I understand it, memoryReferences are never interpreted by the client but they can be used to set breakpoints -- without a need for interpreting. As I understand it, every time the client makes a request of the DAP that requires a memory reference, it is a memory reference previously supplied by the DAP as a response to some request. While I didn't I could have returned addresses that are not even hex-looking numbers -- maybe I should do that --- as in my case, that can be useful as in my device 0xFFFFFFFF is a valid address but there won't be code there.

I can see from the client-side why instructionCount is needed -- gives a virtual feeling to the user that full disassembly is available and scrolling is nice. It was very nicely done and I found it responsive.

[puremourning]

It turns out that returning the exact requested number is fundamentally required in order for clients to use this API. So I'm strongly against this change.

Without the above requirement, it's not possible to work out which instruction relates to the requested memory reference.

[roblourens]

Skimming the original discussion #42 it's not clear to me why that requirement exists. I understand the case @puremourning brings up, but wouldn't it work to say that if the client asks for 100 instructions, and 75 are returned, then you reached the end of the available instructions? I see it as similar to the stacktrace request.

[puremourning]

Skimming the original discussion #42 it's not clear to me why that requirement exists. I understand the case @puremourning brings up, but wouldn't it work to say that if the client asks for 100 instructions, and 75 are returned, then you reached the end of the available instructions? I see it as similar to the stacktrace request.

I mean I guess it works if the adapter is required to return at least (-instructionOffset) instructions in the case of negative offset. (Otherwise requested instruction must be instruction 0 in the response)

But tbh it's not that hard to just pad it like the spec says.

[roblourens]

I guess with a negative offset, you can run off both the front or the end of the valid range, and you either need the DA to pad the response and return the exact requested number, or you need a system for the response to tell the client whether frames are missing from the beginning or end (like a actualOffset flag in the response).

[puremourning]

I guess with a negative offset, you can run off both the front or the end of the valid range, and you either need the DA to pad the response and return the exact requested number, or you need a system for the response to tell the client whether frames are missing from the beginning or end (like a actualOffset flag in the response).

Agree. But given we already have a spec that says it should just be padded. And clients have been implemented based on that... why not just pad it.