public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Nav Mohammed <nav@oscillate.io>
To: gdb-patches@sourceware.org
Subject: [RFC] [PATCH] Improve range stepping efficiency (ensure GDB steps over entire line, at once)
Date: Wed, 20 Sep 2023 22:38:26 +0100	[thread overview]
Message-ID: <CAFYgX4Y35pLeM+oNE7V4MOJ9z66nsOmWTVj77pnBFOuTTGns9g@mail.gmail.com> (raw)


[-- Attachment #1.1: Type: text/plain, Size: 3073 bytes --]

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 Mohammed*
Software Developer

[-- Attachment #2: 0001-cover-entire-line-in-pc-range.patch --]
[-- Type: text/x-patch, Size: 848 bytes --]

diff --git a/gdb/symtab.c b/gdb/symtab.c
index c0c2454d967..31f91a8b7f0 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3196,6 +3196,12 @@ find_pc_sect_line (CORE_ADDR pc, struct obj_section *section, int notcurrent)
       if (item != first)
 	prev = item - 1;		/* Found a matching item.  */
 
+      /* For lines with multiple statements, ensure that `item` points to the next *line*,
+	 as opposed to the next statement on the same line. That way, the PC range we return
+	 will cover the whole line, as opposed to a single statement on the line. */
+      while (item->line == prev->line && item != last)
+		++item;
+
       /* At this point, prev points at the line whose start addr is <= pc, and
 	 item points at the next line.  If we ran off the end of the linetable
 	 (pc >= start of the last line), then prev == item.  If pc < start of

             reply	other threads:[~2023-09-20 21:38 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-20 21:38 Nav Mohammed [this message]
2023-09-21 20:01 ` Tom Tromey

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAFYgX4Y35pLeM+oNE7V4MOJ9z66nsOmWTVj77pnBFOuTTGns9g@mail.gmail.com \
    --to=nav@oscillate.io \
    --cc=gdb-patches@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).