From patchwork Wed Sep 20 21:38:26 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nav Mohammed X-Patchwork-Id: 76467 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5BBCD3857737 for ; Wed, 20 Sep 2023 21:38:55 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mail-vk1-xa31.google.com (mail-vk1-xa31.google.com [IPv6:2607:f8b0:4864:20::a31]) by sourceware.org (Postfix) with ESMTPS id DCC8E3858D1E for ; Wed, 20 Sep 2023 21:38:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DCC8E3858D1E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=oscillate.io Authentication-Results: sourceware.org; spf=none smtp.mailfrom=navnav.me Received: by mail-vk1-xa31.google.com with SMTP id 71dfb90a1353d-49032a0ff13so125820e0c.0 for ; Wed, 20 Sep 2023 14:38:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=navnav-me.20230601.gappssmtp.com; s=20230601; t=1695245918; x=1695850718; darn=sourceware.org; h=to:subject:message-id:date:from:mime-version:from:to:cc:subject :date:message-id:reply-to; bh=SUhhCRzXvf4gnUAgawZnXlBRapUO25f0Y3eJNu6RYS8=; b=v3aPleEAMfT5HLYHuDnbfp/xpvWvbGUgMMPHlqQunU/ozsS9RKiYGBJPDLim32DrEt 6ypJW7GL2rQz9biCDDKVsaP8mxKQKk8AyAgrt7qVJfBeE30GXzy3L9wNU4XvOBUXKWj4 JCfDoC1KZWf+VqETxHyeBLkBSc5JEI4Smk5CDWN0A8YXbGXl2hHEQtb8uhU7NBklO6iD swITTB6TcXSFE7zcYU0pQODw1e4NSPOGPeLwiLH4kg3DEjYJbPKrPoJhBUY6JZKAkBMK YuJOcuCVKIEO6/tPBAw2jrfgZTAMOVwXKyyy8rnn1xGQU13wdsVXNcFJSN8x/Nlubn9z tPZQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695245918; x=1695850718; h=to:subject:message-id:date:from:mime-version:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=SUhhCRzXvf4gnUAgawZnXlBRapUO25f0Y3eJNu6RYS8=; b=OIZHdLJ7hjeAEaMA/3iMLKlJJfquTiHDl22njuj50JhzpUdhnVWvZjq/d9/LqwxaBv absajgtmTD7z4d32r25CZf8iE6AoNBF2PUsqREAdB7tNkgmHF7pC19Y0gbfsGcqC1gw0 QTmUuTVGhj/t0QvmcKHU3k1rrM0ClB6pBY4Ao7nTSwMGpT9chVNmp2+NzTLsT61cs76+ W/P8GkmswgpRzSx5/KsNDc1c+EzNI7DpYA6yobw60M2DyCnVBAevzqtzvY+50+PIb4KZ iuPuzveLvhZLQDNLWqn8bHF7vLMr2/0xoz38u5B/dgz8sJ3aMtEdkm84a+cJDNnA5Tpe aS7Q== X-Gm-Message-State: AOJu0YwiCWEw480ppKkgvd1DeyXio2dXfJPpG0VBmKqlpky1+Cluq03B vujsHtQBZFC3ePw6n8eEl+4wMXaNTVxa8M/cM2Q7R8FWHc7lAYMuAdM= X-Google-Smtp-Source: AGHT+IEpd3oeN4CcQdXrTmaYpE0+vhu9Hy9AxH/hos/GkXoqXNVZ9zbVcUNeXxQEGCYFwfonT0WdCx8KgDmzwU3LFg8= X-Received: by 2002:a1f:cc01:0:b0:48d:3305:d23 with SMTP id c1-20020a1fcc01000000b0048d33050d23mr3829992vkg.2.1695245917652; Wed, 20 Sep 2023 14:38:37 -0700 (PDT) MIME-Version: 1.0 From: Nav Mohammed Date: Wed, 20 Sep 2023 22:38:26 +0100 Message-ID: Subject: [RFC] [PATCH] Improve range stepping efficiency (ensure GDB steps over entire line, at once) To: gdb-patches@sourceware.org X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, HTML_MESSAGE, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-Content-Filtered-By: Mailman/MimeDel 2.1.30 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" 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. 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