[PATCH v3] gdb/DAP Fix disassemble bug

[PATCH v3] gdb/DAP Fix disassemble bug

[Simon Farre]

v3.

This adds the ability to "disassemble backwards" by retrieving "known
sources of truth" by using the gdb.Block data type, Block.start is always a
valid position in executable code, no? If not, then gdb.Block and all
code that relates to gdb.Block and it's underlying type, must also be
invalid, so I assume here that Block.start is always "good", as it were.

It would probably be nice to have a test for this if we don't already,
but from my dog fooding experience with Midas+VSCode+GDB-DAP, it *seems* to be
correct. But looks can be deceiving, I guess.

v1.
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 | 44 +++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/disassemble.py b/gdb/python/lib/gdb/dap/disassemble.py
index bc091eb2c89..ce016067b98 100644
--- a/gdb/python/lib/gdb/dap/disassemble.py
+++ b/gdb/python/lib/gdb/dap/disassemble.py
@@ -18,6 +18,39 @@ import gdb
 from .server import request, capability
 from .startup import send_gdb_with_response, in_gdb_thread
 
+# Disassemble "backwards"
+def disasm_backwards(arch: gdb.Architecture, end_pc: int, offset: int, count: int):
+    ins_at_pc = arch.disassemble(start_pc=end_pc)[0]
+    offset = abs(offset)
+    # We take an arbitrary jump backwards
+    # Guessing that an instruction averages at 8 bytes
+    start = end_pc - 8 * offset
+    instructions = []
+    while len(instructions) < (offset + 1):
+        block = gdb.current_progspace().block_for_pc(start)
+        # Fail fast; if we can't find a block backwards
+        # fill all with "invalid values"
+        if block is None:
+            tmp = []
+            for i in range(0, offset - len(instructions)):
+                tmp.append({"addr": 0, "asm": "unknown"})
+            instructions = tmp + instructions
+        else:
+            ins = arch.disassemble(start_pc=block.start, end_pc=end_pc)
+            instructions = ins + instructions
+        start = start - 8 * (offset - len(instructions))
+        end_pc = block.start
+
+    # Disassembled instructions count is >= OFFSET+1
+    diff = len(instructions) - offset
+    result = instructions[diff : diff + count]
+    # DAP seem to not want the actual instruction *at* end_pc
+    # when disassembling backwards
+    if result[-1]["addr"] == ins_at_pc["addr"]:
+        result.pop()
+        result = [instructions[diff - 1]] + result
+    return result[:count]
+
 
 @in_gdb_thread
 def _disassemble(pc, skip_insns, count):
@@ -27,8 +60,15 @@ 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:]:
+    if skip_insns < 0:
+        ins = disasm_backwards(arch, pc, skip_insns, count)
+        skip_insns = 0
+        count = count - len(ins)
+        result = [
+            {"address": hex(elt["addr"]), "instruction": elt["asm"]} for elt in ins
+        ]
+
+    for elt in arch.disassemble(pc, count=count + skip_insns)[skip_insns:]:
         result.append(
             {
                 "address": hex(elt["addr"]),
-- 
2.41.0

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

[Tom Tromey]

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

Simon>  from .startup import send_gdb_with_response, in_gdb_thread
 
Simon> +# Disassemble "backwards"
Simon> +def disasm_backwards(arch: gdb.Architecture, end_pc: int, offset: int, count: int):

A few nits here:

* Should use @in_gdb_thread

* I can tell from the line spacing that you didn't run "black".

* The DAP code has a convention of prefixing internal functions with "_"

Simon> +    while len(instructions) < (offset + 1):
Simon> +        block = gdb.current_progspace().block_for_pc(start)
Simon> +        # Fail fast; if we can't find a block backwards
Simon> +        # fill all with "invalid values"
Simon> +        if block is None:
Simon> +            tmp = []
Simon> +            for i in range(0, offset - len(instructions)):
Simon> +                tmp.append({"addr": 0, "asm": "unknown"})
Simon> +            instructions = tmp + instructions

I wonder if this block should end with a 'break'.  I guess I don't
really follow the logic here.

Simon> +    if skip_insns < 0:
Simon> +        ins = disasm_backwards(arch, pc, skip_insns, count)
Simon> +        skip_insns = 0
Simon> +        count = count - len(ins)
Simon> +        result = [
Simon> +            {"address": hex(elt["addr"]), "instruction": elt["asm"]} for elt in ins
Simon> +        ]
Simon> +
Simon> +    for elt in arch.disassemble(pc, count=count + skip_insns)[skip_insns:]:

Likewise it seems weird to fall through here.

I think a test case with a negative skip would be good.

Tom

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

[Simon Farre]

> A few nits here:
>
> * Should use @in_gdb_thread
>
> * I can tell from the line spacing that you didn't run "black".
>
> * The DAP code has a convention of prefixing internal functions with "_"
I'll be sure to fix the nits!
> Simon> +    while len(instructions) < (offset + 1):
> Simon> +        block = gdb.current_progspace().block_for_pc(start)
> Simon> +        # Fail fast; if we can't find a block backwards
> Simon> +        # fill all with "invalid values"
> Simon> +        if block is None:
> Simon> +            tmp = []
> Simon> +            for i in range(0, offset - len(instructions)):
> Simon> +                tmp.append({"addr": 0, "asm": "unknown"})
> Simon> +            instructions = tmp + instructions
>
> I wonder if this block should end with a 'break'.  I guess I don't
> really follow the logic here.
That's probably true, as that would break the while loop's condition, so 
we might as well
break out early.
> Simon> +    if skip_insns < 0:
> Simon> +        ins = disasm_backwards(arch, pc, skip_insns, count)
> Simon> +        skip_insns = 0
> Simon> +        count = count - len(ins)
> Simon> +        result = [
> Simon> +            {"address": hex(elt["addr"]), "instruction": elt["asm"]} for elt in ins
> Simon> +        ]
> Simon> +
> Simon> +    for elt in arch.disassemble(pc, count=count + skip_insns)[skip_insns:]:
>
> Likewise it seems weird to fall through here.
>
> I think a test case with a negative skip would be good.
>
> Tom

So, this block explicitly is what solves the problem with the prior 
patch (where skip_insns could be negative, but wasn't accounted for - 
particularly if total count was more than that negative offset)
Let's take a simple example, where offset is -50 and total count is 100, 
that means we want instructions in the range [-50 .. 50);
If skip_insns is less than 0 (i.e., the instructionOffset is negative), 
we deal with those first - set skip_insns to 0 and decrement total count 
of requested instructions
Then we proceed and disassemble the remainder in the "postive offset 
direction". If there is no remainder after the first branch, no more 
instructions are going to be dissassembled.

The reason why I'm setting skip_insns = 0 here, is to not have strange 
and long if-then-else branches. Because then we must account for a 
multiplication of things (instead of the simple fall-through approach);
The different scenarios would be:
Is offset negative? * (is total count less than that offset? || is total 
count more than that offset?) = 1 + (2)
Is offset positive? (total count doesn't matter, we just start at offset 
.. offset+total count) = 1
Where each branch also will have copy pasted logic to account for these 
different scenarios.

Here we instead just say "process before, then process what remains" - 
if skip_insns (instructionOffset) isn't negative, that branch is never 
taken.
I think perhaps the current checked in work, confuses things with how 
it's named in the helper function;
skip_insns is actually not skipped instructions it's instructionOffset 
as you can see in the actual handler function.

I would like some pointers though, on how we would test this? Decide on 
a platform?
Because it's not just a matter of testing the instruction count being 
returned, that's trivial, we must also make sure that the *actual 
disassembly* is correct.
If the test suite was written in a "good" (don't let my frustration at 
Tcl shine through:P) language, I could write something that calls 
objdump/readelf, reads the actual instructions
and compares it with the output of the request. In Python or Javascript 
this would be trivial and could be whipped up in 10 minutes, I am not as 
confident it's that easy in Tcl unfortunately.
Perhaps call into Python from tcl? What do you think?

Either way, next patch will have tests on the instruction count being 
correct at least.