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

[Simon Farre <simon.farre.cx@gmail.com>]

> 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.