[RFC] Improve range stepping efficiency (ensure GDB steps over entire line, at once)

  Hi all,

First time contributor to GDB, please go easy on me.

I've been implementing support for range stepping (vCont;r packets) in a
GDB stub. I've noticed an issue with GDB, where it performs multiple range
steps when stepping over a single line of code.

I've narrowed this down to the implementation of the find_pc_sect_line()
function in symtab.c. The comment for that function explicitly states that
the returned PC range should cover the entire line: "Return a structure
containing a symtab pointer, a line number, and a pc range for the entire
source line.". But what I've found is that it's only returning the PC range
for a particular *statement *on the line, not the whole line. This is
resulting in GDB issuing multiple range steps for what should be a single
range step.

Consider the following lines of code from a simple AVR program:

DDRD = 0xFF;  // Line 41
PORTD = 0x00; // Line 42
PORTD = 0x01; // Line 43

If we dump the DWARF line table info for those lines (via
readelf --debug-dump=decodedline)

main.c                                        41               0x31e
 50       x
main.c                                        41               0x322
 51       x
main.c                                        42               0x328
 52       x
main.c                                        42               0x32c
 53       x
main.c                                        43               0x330
 54       x
main.c                                        43               0x334
 55       x

Note that are two line entries for all three lines.

Now, if we debug this program with GDB, with the current PC at 0x328
(beginning of line 42), and trigger a step-over with the `next` command,
GDB will issue *two* range steps, one for each line entry. This is because
the call to find_pc_sect_line() returns the PC range for only the
first *statement
*on line 42, not the whole line. So it returns 0x328 -> 0x32c when it
should be returning 0x328 -> 0x330.

Why is this a problem?
The GDB stub I maintain is primarily for AVR targets, where there's quite a
lot of overhead when accessing target memory, setting breakpoints, etc,
most of which GDB will do after each range step. So this results in a
noticeable delay of about one second, for each step-over.

I've attached a patch to address this issue, but in all honesty, it's a bit
of a hacky workaround where I've tried to keep the number of changed lines
to an absolute minimum. This is because I'm totally new to the GDB codebase
and I don't want to introduce more bugs by making too many changes. And
given that find_pc_sect_line() is called in quite a few places, I'm
concerned that some of those may *expect* the old behaviour from that
function (return a PC range for a single statement).

I've tested the patch and it solves the issue, but I'd like someone who's
familiar with this area of the codebase to review the patch. And if you
think there's a better way to do it, please let me know and I'll give it a
go.

Apologies if I've misunderstood something here.
  

>>>>> "Nav" == Nav Mohammed <nav@oscillate.io> writes:

Nav> First time contributor to GDB, please go easy on me.

Hi, and welcome to gdb.

Thank you for the patch.

Nav> I've attached a patch to address this issue, but in all honesty, it's a bit
Nav> of a hacky workaround where I've tried to keep the number of changed lines
Nav> to an absolute minimum. This is because I'm totally new to the GDB codebase
Nav> and I don't want to introduce more bugs by making too many changes. And
Nav> given that find_pc_sect_line() is called in quite a few places, I'm
Nav> concerned that some of those may *expect* the old behaviour from that
Nav> function (return a PC range for a single statement).

I have this same fear.  I don't really understand this function too
well, and I tend to think that somebody else ought to review it.

One thought that occurred to me, though, is that it would be good to
have a test case.  A unit test seems maybe simplest/best, but that seems
like a real pain, though, since the function reaches into the minimal
symbols, blockvector, etc.

Maybe the body of the central loop could be pulled into a separate
function and then that could be unit-tested?  It would only rely on line
tables, and making test line tables seems pretty easy.

I wonder if you have any thoughts on how to test it.

Tom