From patchwork Tue Feb 11 15:39:23 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 37969 Received: (qmail 111175 invoked by alias); 11 Feb 2020 15:39:30 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 111166 invoked by uid 89); 11 Feb 2020 15:39:29 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mail-wr1-f68.google.com Received: from mail-wr1-f68.google.com (HELO mail-wr1-f68.google.com) (209.85.221.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 11 Feb 2020 15:39:27 +0000 Received: by mail-wr1-f68.google.com with SMTP id g3so11845486wrs.12 for ; Tue, 11 Feb 2020 07:39:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=1kbnaN378b6Rys7lXjOQ1AMmWetxaSvl+4jIiQrHEvw=; b=VCErabRPnOmB/ASQGi4rrqQ5yFVQ01sfRlwZIEQXVJO46c9nlTDDgVWwZzwCUpj3+l xUwZh8QyUTtQ8W/iFNscopmwCvf0n0H/WrZXJvkpOqC+1K8axlcWF9MAX5PnYVXkpRih TNiLz7YGDKsBDjg92wmmF/2XifyuUZyBSL2luQv8hqIA2XQ+MEDi6bSXKxLTwPvntUtY UgQoMn/ygmCzVjInwOxbSTeX8dhPG7CWkOT9NcEoQFSD4G4Hc/nXeqzJTGbWkiOjXoTT rDwHlN9qUHRXNuIL+9Hpfpp15Lyx/UgBAMuLoAQyKLWuqXz/whVIpcKYD10OOfmGUeFE yAsQ== Return-Path: Received: from localhost (host81-151-181-192.range81-151.btcentralplus.com. [81.151.181.192]) by smtp.gmail.com with ESMTPSA id y131sm4313189wmc.13.2020.02.11.07.39.24 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 11 Feb 2020 07:39:24 -0800 (PST) Date: Tue, 11 Feb 2020 15:39:23 +0000 From: Andrew Burgess To: Luis Machado Cc: gdb-patches@sourceware.org, Bernd Edlinger Subject: Re: [PATCH 2/2] gdb: Add support for tracking the DWARF line table is-stmt field Message-ID: <20200211153923.GS4020@embecosm.com> References: <94e33268f64060fc887670f4ee5ed524050cbcc7.1580902412.git.andrew.burgess@embecosm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Fortune: When you're down and out, lift up your voice and shout, "I'M DOWN AND OUT"! X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes * Luis Machado [2020-02-06 06:00:29 -0300]: > Hi, > > I still need to check the patch itself, but i had a question about one > particular paragraph... > > On 2/5/20 8:37 AM, Andrew Burgess wrote: > > This commit brings support for the DWARF line table is_stmt field to > > GDB. The is_stmt field is used by the compiler when a single source > > line is split into multiple assembler instructions, especially if the > > assembler instructions are interleaved with instruction from other > > source lines. > > > > The compiler will set the is_stmt flag false from some instructions > > from the source lines, these instructions are not a good place to > > insert a breakpoint in order to stop at the source line. > > Instructions which are marked with the is_stmt flag true are a good > > place to insert a breakpoint for that source line. > > > > Currently GDB ignores all instructions for which is_stmt is false. > > This is fine in a lot of cases, however, there are some cases where > > this means the debug experience is not as good as it could be. > > > > Consider stopping at a random instruction, currently this instruction > > will be attributed to the last line table entry before this point for > > which is_stmt was true - as these are the only line table entries that > > GDB tracks. This can easily be incorrect in code with even a low > > level of optimisation. > > > > With is_stmt tracking in place, when stopping at a random instruction > > we now attribute the instruction back to the real source line, even > > when is_stmt is false for that instruction in the line table. > > > > When inserting breakpoints we still select line table entries for > > which is_stmt is true, so the breakpoint placing behaviour should not > > change. > > > > When stepping though code (at the line level, not the instruction > > level) we will still stop at instruction where is_stmt is true, I > > think this is more likely to be the desired behaviour. > > Considering a block of continuous instructions that map to the same source > line, will line-stepping stop at each one of these instructions if is_stmt > is true? As opposed to stepping over the the whole block until we see a line > change? No, a continuous block will be skipped as it is at the moment, even if is_stmt is true for all entries. > > I'm wondering if this would help this bug > (https://sourceware.org/bugzilla/show_bug.cgi?id=21221) in any way. I took a look at this bug and even had a few ideas on how we might improve GDB. Below is one patch that I put together, though this only fixes one of the example cases from that bug. The problem this patch runs into is that it regresses a few GDB tests (gdb.base/gdb-sigterm.exp and gdb.thread/step-bg-decr-pc-switch-thread.exp) in at least one of these tests GDB makes the following assumption: Single stepping on a one line loop (e.g. 'for (;;);') will block forever. The question then is: Does single step move us to the next source line to be executed that is not the current line, or does single step move us to the next source line to be executed, even if it is the same as the current line? I'm not sure what the answer is to this question... Thanks, Andrew --- commit 35a72ddacf86c7e7f899c869558d1af3cfadb549 Author: Andrew Burgess Date: Tue Feb 11 15:08:10 2020 +0000 WIP: Try to better handle small loops This handles this case: int main(void) { int var = 0; for (;;) { var++; } return 0; } Compiled as 'gcc -g -o test.x test.c'. Change-Id: I6f499181eca5fb51b6edb050cbce0bda15665d38 diff --git a/gdb/infrun.c b/gdb/infrun.c index 3919de81f90..03878025959 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -6674,11 +6674,14 @@ process_event_stop_test (struct execution_control_state *ecs) /* When stepping backward, stop at beginning of line range (unless it's the function entry point, in which case - keep going back to the call point). */ + keep going back to the call point). When stepping forward we + stop at the beginning of the range as this indicates we must be + looping. */ CORE_ADDR stop_pc = ecs->event_thread->suspend.stop_pc; if (stop_pc == ecs->event_thread->control.step_range_start - && stop_pc != ecs->stop_func_start - && execution_direction == EXEC_REVERSE) + && (execution_direction != EXEC_REVERSE + || (stop_pc != ecs->stop_func_start + && execution_direction == EXEC_REVERSE))) end_stepping_range (ecs); else keep_going (ecs); @@ -7173,6 +7176,16 @@ process_event_stop_test (struct execution_control_state *ecs) } } + if (ecs->event_thread->suspend.stop_pc + == ecs->event_thread->control.step_range_start) + { + /* Very small loops might only contain a single line information + entry. If this is the case then spot when we return to the start + of the line range and stop again. */ + end_stepping_range (ecs); + return; + } + /* We aren't done stepping. Optimize by setting the stepping range to the line.