From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id B42D0393BC21 for ; Tue, 13 Jul 2021 02:53:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B42D0393BC21 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 16D2r7mC010900 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 12 Jul 2021 22:53:12 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 16D2r7mC010900 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id DACFE1E79C; Mon, 12 Jul 2021 22:53:07 -0400 (EDT) Subject: Re: [PATCH 3/3] Fix reverse stepping multiple contiguous PC ranges To: Luis Machado , gdb-patches@sourceware.org References: <20210201191930.2696351-1-luis.machado@linaro.org> From: Simon Marchi Message-ID: <5791c5b6-face-f7ad-2a41-5de570619895@polymtl.ca> Date: Mon, 12 Jul 2021 22:53:07 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210201191930.2696351-1-luis.machado@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Tue, 13 Jul 2021 02:53:08 +0000 X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Jul 2021 02:53:16 -0000 On 2021-02-01 2:19 p.m., Luis Machado via Gdb-patches wrote: > When running GDB's testsuite on aarch64-linux/Ubuntu 20.04, I noticed some > failures in gdb.reverse/solib-precsave.exp and gdb.reverse/solib-reverse.exp. > > The failure happens around the following code: > > 38 b[1] = shr2(17); /* middle part two */ > 40 b[0] = 6; b[1] = 9; /* generic statement, end part two */ > 42 shr1 ("message 1\n"); /* shr1 one */ > > Normal execution: > > - step from line 1 will land on line 2. > - step from line 2 will land on line 3. > > Reverse execution: > > - step from line 3 will land on line 2. > - step from line 2 will land on line 2. > - step from line 2 will land on line 1. > > The problem here is that line 40 contains two contiguous but distinct > PC ranges, like so: > > Line 40 - [0x7ec ~ 0x7f4] > Line 40 - [0x7f4 ~ 0x7fc] > > When stepping forward from line 2, we skip both of these ranges and land on > line 42. When stepping backward from line 3, we stop at the start PC of the > second (or first, going backwards) range of line 40. > > This happens because we have this check in infrun.c:process_event_stop_test: > > /* 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). */ > 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) > end_stepping_range (ecs); > else > keep_going (ecs); > > Since we've reached ecs->event_thread->control.step_range_start, we stop > stepping backwards. > > The right thing to do is to look for adjacent PC ranges for the same line, > until we notice a line change. Then we take that as the start PC of the > range. > > Another solution I thought about is to merge the contiguous ranges when > we are reading the line tables. Though I'm not sure if we really want to process > that data as opposed to keeping it as the compiler created, and then working > around that. > > In any case, the following patch addresses this problem. > > I'm not particularly happy with how we go back in the ranges (using "pc - 1"). > Feedback would be welcome. I don't have a deep knowledge of this area, so I'd like to know, how does it work for forward stepping over the example you gave above? What is the sequence of decisions that lead to GDB deciding to continue stepping more? Because I am thinking that what we do for reverse stepping should be pretty much the same as what we do for forward stepping, just the other way around. > Validated on aarch64-linux/Ubuntu 20.04/18.04. > > Ubuntu 18.04 doesn't actually run into these failures because the compiler > doesn't generate distinct PC ranges for the same line. Could it be that the new compiler adds column information? That would explain why it's now two ranges. > gdb/ChangeLog: > > YYYY-MM-DD Luis Machado > > * infrun.c (process_event_stop_test): Handle backward stepping > across multiple ranges for the same line. > * symtab.c (find_line_range_start): New function. > * symtab.h (find_line_range_start): New prototype. > --- > gdb/infrun.c | 22 +++++++++++++++++++++- > gdb/symtab.c | 35 +++++++++++++++++++++++++++++++++++ > gdb/symtab.h | 16 ++++++++++++++++ > 3 files changed, 72 insertions(+), 1 deletion(-) > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index e070eff33d..5bb268529d 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -6534,11 +6534,31 @@ process_event_stop_test (struct execution_control_state *ecs) > have software watchpoints). */ > ecs->event_thread->control.may_range_step = 1; > > + /* When we are stepping inside a particular line range, in reverse, > + and we are sitting at the first address of that range, we need to > + check if this address also shows up in another line range as the > + end address. > + > + If so, we need to check what line such a step range points to. > + If it points to the same line as the current step range, that > + means we need to keep going in order to reach the first address > + of the line range. We repeat this until we eventually get to the > + first address of a particular line we're stepping through. */ > + CORE_ADDR range_start = ecs->event_thread->control.step_range_start; > + if (execution_direction == EXEC_REVERSE) > + { > + gdb::optional real_range_start > + = find_line_range_start (ecs->event_thread->suspend.stop_pc); With what I just merged, you'll find that you need to replace ->suspend.stop_pc with `->stop_pc ()`. Nothing major. > + > + if (real_range_start.has_value ()) > + range_start = *real_range_start; The question I have while reading this is: what did set the original range_start, ecs->event_thread->control.step_range_start? Why didn't we get the "real" range start at the time and have to fix it up now? > + } > + > /* 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). */ > CORE_ADDR stop_pc = ecs->event_thread->suspend.stop_pc; > - if (stop_pc == ecs->event_thread->control.step_range_start > + if (stop_pc == range_start > && stop_pc != ecs->stop_func_start > && execution_direction == EXEC_REVERSE) > end_stepping_range (ecs); > diff --git a/gdb/symtab.c b/gdb/symtab.c > index 7ffb52a943..e8f5301951 100644 > --- a/gdb/symtab.c > +++ b/gdb/symtab.c > @@ -3382,6 +3382,41 @@ find_pc_line (CORE_ADDR pc, int notcurrent) > return sal; > } > > +/* See symtah.h. */ > + > +gdb::optional > +find_line_range_start (CORE_ADDR pc) > +{ > + struct symtab_and_line current_sal = find_pc_line (pc, 0); > + > + if (current_sal.line == 0) > + return {}; > + > + struct symtab_and_line prev_sal = find_pc_line (current_sal.pc - 1, 0); > + > + /* If the previous entry is for a different line, that means we are already > + at the entry with the start PC for this line. */ > + if (prev_sal.line != current_sal.line) > + return current_sal.pc; > + > + /* Otherwise, keep looking for entries for the same line but with > + smaller PC's. */ > + bool done = false; > + CORE_ADDR prev_pc; > + while (!done) > + { > + prev_pc = prev_sal.pc; > + > + prev_sal = find_pc_line (prev_pc - 1, 0); > + > + /* Did we notice a line change? If so, we are done with the search. */ > + if (prev_sal.line != current_sal.line) > + done = true; > + } > + > + return prev_pc; Above, should we compare more than just the line? What I am thinking is the edge case where looking up PC would return an SAL, say `a.c:40`, and looking up "PC - 1" would return `b.c:40`. The algorithm would think that we are still on the same line and keep searching, when in fact it's a different line 40, so we should have stopped. Simon