[PATCH v1] gdb/DAP Fix disassemble bug

[PATCH v1] gdb/DAP Fix disassemble bug

[Simon Farre]

Fixes disassembleRequest

The field instructionOffset can be negative. Previous patch made it so
that sometimes the request got calculated to 0 instructions, when it
meant to retrieve disasm for -50 to 0 (current position).
---
 gdb/python/lib/gdb/dap/disassemble.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/disassemble.py b/gdb/python/lib/gdb/dap/disassemble.py
index bc091eb2c89..1ee6cf2ff48 100644
--- a/gdb/python/lib/gdb/dap/disassemble.py
+++ b/gdb/python/lib/gdb/dap/disassemble.py
@@ -27,8 +27,8 @@ def _disassemble(pc, skip_insns, count):
         # Maybe there was no frame.
         arch = gdb.selected_inferior().architecture()
     result = []
-    total_count = skip_insns + count
-    for elt in arch.disassemble(pc, count=total_count)[skip_insns:]:
+
+    for elt in arch.disassemble(pc + skip_insns, count=count):
         result.append(
             {
                 "address": hex(elt["addr"]),
-- 
2.41.0

Re: [PATCH v1] gdb/DAP Fix disassemble bug

[Tom Tromey]

>>>>> "Simon" == Simon Farre via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Fixes disassembleRequest
Simon> The field instructionOffset can be negative. Previous patch made it so
Simon> that sometimes the request got calculated to 0 instructions, when it
Simon> meant to retrieve disasm for -50 to 0 (current position).

I don't think this will work correctly, because this isn't counting by
instruction but rather by byte.

instructionOffset is defined in terms of instructions:

    Offset (in instructions) to be applied after the byte offset (if any)
    before disassembling. Can be negative.

I must have missed the "negative" note, or maybe I just ignored it
without documenting that -- since I wonder how it can possibly work.  it
seems to me that on architectures with variable length instructions, you
can't really disassemble in "reverse" like that.

I guess one idea would be to back up to the previous symbol and start
disassembling from there.  I feel like the TUI did this, though, and ran
into all kinds of weird corner cases.

Simon> -    for elt in arch.disassemble(pc, count=total_count)[skip_insns:]:

I notice now that the current code also neglects this part of the spec:

   * An adapter must return exactly this number of instructions - any
   * unavailable instructions should be replaced with an implementation-defined
   * 'invalid instruction' value.

Tom

Re: [PATCH v1] gdb/DAP Fix disassemble bug

[Simon Farre]

On 6/26/23 20:34, Tom Tromey wrote:
>>>>>> "Simon" == Simon Farre via Gdb-patches <gdb-patches@sourceware.org> writes:
> Simon> Fixes disassembleRequest
> Simon> The field instructionOffset can be negative. Previous patch made it so
> Simon> that sometimes the request got calculated to 0 instructions, when it
> Simon> meant to retrieve disasm for -50 to 0 (current position).
>
> I don't think this will work correctly, because this isn't counting by
> instruction but rather by byte.
>
> instructionOffset is defined in terms of instructions:
>
>      Offset (in instructions) to be applied after the byte offset (if any)
>      before disassembling. Can be negative.
>
> I must have missed the "negative" note, or maybe I just ignored it
> without documenting that -- since I wonder how it can possibly work.  it
> seems to me that on architectures with variable length instructions, you
> can't really disassemble in "reverse" like that.
>
> I guess one idea would be to back up to the previous symbol and start
> disassembling from there.  I feel like the TUI did this, though, and ran
> into all kinds of weird corner cases.
>
> Simon> -    for elt in arch.disassemble(pc, count=total_count)[skip_insns:]:
>
> I notice now that the current code also neglects this part of the spec:
>
>     * An adapter must return exactly this number of instructions - any
>     * unavailable instructions should be replaced with an implementation-defined
>     * 'invalid instruction' value.
>
> Tom


Then perhaps we should just ignore any and all offsets and *just* care 
about count and produce current addr + N instructions and resolve 
"invalid value" for the rest.

Re: [PATCH v1] gdb/DAP Fix disassemble bug

[Simon Farre]

Also, I don't know what design GDB has, but this kind of information is 
probably knowable by utilizing DWARF. Just guessing here, but it seems 
like if one were to use the line number program in dwarf, being able to 
"disassemble backwards" should be fairly easy. That kind of would have 
to require that the current design is suitable for that and the little 
I've looked at it I don't think this is the case, unfortunately. I'll 
update the patch by just returning "invalid values" prior to the 
"current pc".

Re: [PATCH v1] gdb/DAP Fix disassemble bug

[Simon Farre]

I've got a working "backwards disassemble" patch, coming soon.

On 6/27/23 00:11, Simon Farre wrote:
> Also, I don't know what design GDB has, but this kind of information 
> is probably knowable by utilizing DWARF. Just guessing here, but it 
> seems like if one were to use the line number program in dwarf, being 
> able to "disassemble backwards" should be fairly easy. That kind of 
> would have to require that the current design is suitable for that and 
> the little I've looked at it I don't think this is the case, 
> unfortunately. I'll update the patch by just returning "invalid 
> values" prior to the "current pc".