From: Bruno Larsen <blarsen@redhat.com>
To: Carl Love <cel@us.ibm.com>,
gdb-patches@sourceware.org, luis.machado@arm.com
Cc: Rogerio Alves <rogealve@br.ibm.com>,
Will Schmidt <will_schmidt@vnet.ibm.com>
Subject: Re: [PATCH V2] Updated, fix reverse stepping multiple contiguous PC ranges
Date: Wed, 23 Mar 2022 09:20:12 -0300 [thread overview]
Message-ID: <36e66d67-6593-b5c2-64e5-487242a40798@redhat.com> (raw)
In-Reply-To: <90f842721dc87da4052ae860c8ee09286dae0012.camel@us.ibm.com>
On 3/22/22 14:05, Carl Love wrote:
> Bruno, GDB maintainers:
>
> I was asked to clean up the message a bit to clarifiy the message.
> Changing this to version 2 since it now has an additional test added to
> the patch and a code fix.
>
> The following patch was posted by Luis Machado on 2/1/2021. There was
> a little discussion on the patch but it was never fully reviewed and
> approved. Luis has a new email address: luis.machado@arm.com.
>
> As of 2/21/2022 the patch did not compile. I made a small fix to get
> it to compile. I commented out the original line in gdb/infrun.c and
> added a new line with the fix and the comment //carll fix to indicate
> what I changed. Clearly the comment needs to be removed if the patch
> is accepted but I wanted to show what I changed.
>
> On Tue, 2022-03-08 at 17:21 -0300, Bruno Larsen wrote:
>> Hello Carl!
>>
>> Thanks for looking at this. Since I don't test on aarch64 often, I am
>> not sure if I see regressions or racy testcases, but it does fix the
>> issue you mentioned, and there doesn't seem to be regressions on
>> x86_64 hardware. I have a few nits, but the main feedback is: could
>> you add a testcase for this, using the dwarf assembler and manually
>> creating contiguous PC ranges, so we can confirm that this is not
>> regressed in the future on any hardware?
>>
>> Also, I can't approve a patch, but with the testcase this patch is
>> mostly ok by me
>
> The attached patch includes a stand alone DWARF test to verify the
> patch works. The test has been verified on i386 and Powerpc.
> Additionally the two lines in infrun.c were combined onto one line as
> Bruno mentioned.
>
> As mentioned Bruno can not approve the patch. Hopefully on of the GDB
> maintainers can give us an additional review to let us know if the
> patch is acceptable. Thanks.
Hi Carl,
I think you forgot to add the testcase to the email. Also a gentle reminder for you to remove the "//carl fix" comment and the commented line of code above.
>
> Carl Love
>
> ------------------------------------------------
> Fix reverse stepping multiple contiguous PC ranges
>
> The following patch fixes 5 test failures in gdb.reverse/solib-precsave.exp
> and 5 test failures in gdb.reverse/solib-reverse.exp. Both testst pass on
> Intel 64-bit. The two tests solib-precsave.exp and solib-reverse.exp both
> initially passed on Intel without the patch.
>
> No additional regression failures were seen with the patch.
>
> A new testcase was added using DWARF statements to reproduce the issue as
> described below. The new testcase has been tested on x86 and Powerpc with
> the patch to that the test case fails without the patch. The issue is fixed
> on both platforms with the patch.
>
> 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.
>
> 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.
>
> 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.
>
> Co-authored-by: Carl Love <cel@us.ibm.com>
> ---
> gdb/infrun.c | 24 +++++++++++++++++++++++-
> gdb/symtab.c | 35 +++++++++++++++++++++++++++++++++++
> gdb/symtab.h | 16 ++++++++++++++++
> 3 files changed, 74 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 0e819553aeb..0fcfd0fb10e 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -6779,11 +6779,33 @@ if (ecs->event_thread->control.proceed_to_finish
> 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<CORE_ADDR> real_range_start
> + // = find_line_range_start (ecs->event_thread->suspend.stop_pc);
> + = find_line_range_start (ecs->event_thread->stop_pc()); //carll fix
> +
> +
> + if (real_range_start.has_value ())
> + range_start = *real_range_start;
> + }
> +
> /* 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->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 a867e1db9fd..600006c7843 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -3425,6 +3425,41 @@ find_pc_line (CORE_ADDR pc, int notcurrent)
> return sal;
> }
>
> +/* See symtah.h. */
> +
> +gdb::optional<CORE_ADDR>
> +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;
> +}
> +
> /* See symtab.h. */
>
> struct symtab *
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index d12eee6e9d8..4d893a8a3b8 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -2149,6 +2149,22 @@ extern struct symtab_and_line find_pc_line (CORE_ADDR, int);
> extern struct symtab_and_line find_pc_sect_line (CORE_ADDR,
> struct obj_section *, int);
>
> +/* Given PC, and assuming it is part of a range of addresses that is part of a
> + line, go back through the linetable and find the starting PC of that
> + line.
> +
> + For example, suppose we have 3 PC ranges for line X:
> +
> + Line X - [0x0 - 0x8]
> + Line X - [0x8 - 0x10]
> + Line X - [0x10 - 0x18]
> +
> + If we call the function with PC == 0x14, we want to return 0x0, as that is
> + the starting PC of line X, and the ranges are contiguous.
> +*/
> +
> +extern gdb::optional<CORE_ADDR> find_line_range_start (CORE_ADDR pc);
> +
> /* Wrapper around find_pc_line to just return the symtab. */
>
> extern struct symtab *find_pc_line_symtab (CORE_ADDR);
--
Cheers!
Bruno Larsen
next prev parent reply other threads:[~2022-03-23 12:20 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-23 16:39 [PATCH] " Carl Love
2022-02-28 18:02 ` Carl Love
2022-03-08 20:21 ` Bruno Larsen
2022-03-08 22:01 ` Carl Love
2022-03-09 12:23 ` Bruno Larsen
2022-03-09 20:52 ` Carl Love
2022-03-10 13:49 ` Bruno Larsen
2022-03-09 14:53 ` Luis Machado
2022-03-10 11:13 ` Luis Machado
2022-03-10 13:19 ` Bruno Larsen
2022-03-10 13:33 ` Luis Machado
2022-03-22 15:28 ` Carl Love
2022-03-22 17:05 ` [PATCH V2] " Carl Love
2022-03-22 17:10 ` Luis Machado
2022-03-23 12:20 ` Bruno Larsen [this message]
2022-03-23 15:43 ` [PATCH V3] " Carl Love
2022-03-31 13:52 ` [PATCH, v2] Fix reverse stepping multiple contiguous PC ranges over the line table Luis Machado
2022-04-04 16:55 ` will schmidt
2022-04-05 8:36 ` Luis Machado
2022-04-05 15:15 ` will schmidt
2022-04-05 15:24 ` Luis Machado
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=36e66d67-6593-b5c2-64e5-487242a40798@redhat.com \
--to=blarsen@redhat.com \
--cc=cel@us.ibm.com \
--cc=gdb-patches@sourceware.org \
--cc=luis.machado@arm.com \
--cc=rogealve@br.ibm.com \
--cc=will_schmidt@vnet.ibm.com \
/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).