From: Bruno Larsen <blarsen@redhat.com>
To: Luis Machado <luis.machado@arm.com>, Carl Love <cel@us.ibm.com>,
gdb-patches@sourceware.org
Cc: Rogerio Alves <rogealve@br.ibm.com>, nd@arm.com
Subject: Re: [PATCH] Updated, fix reverse stepping multiple contiguous PC ranges
Date: Thu, 10 Mar 2022 10:19:49 -0300 [thread overview]
Message-ID: <49264d6f-d66c-774e-f57b-9eb2efd767d5@redhat.com> (raw)
In-Reply-To: <81c5bece-012b-cad6-8349-71bb255ff242@arm.com>
On 3/10/22 08:13, Luis Machado wrote:
> Hi!
>
> On 3/8/22 20:21, Bruno Larsen via Gdb-patches wrote:
>> On 2/23/22 13:39, Carl Love via Gdb-patches wrote:
>>>
>>> GCC maintainers:
>>>
>>> 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. The email for Luis <luis.machado@linaro.org> no longer
>>> works.
>>>
>>> As of 2/21/2022, the patch does 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.
>>>
>>> That said, I tested the patch on Powerpc and it fixed the 5 test
>>> failures in gdb.reverse/solib-precsave.exp and 5 test failures in
>>> gdb.reverse/solib-reverse.exp. I tested on Intel 64-bit. The two
>>> tests solib-precsave.exp and solib-reverse.exp both initially passed on
>>> Intel. No additional regression failures were seen with the patch.
>>>
>>> Please let me know if you have comments on the patch or if it is
>>> acceptable as is. Thank you.
>>>
>>> Carl Love
>>
>> 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
>>
>>> -----------------------------------------------------------
>>> From: Luis Machado <luis.machado@linaro.org>
>>> Date: Mon, 21 Feb 2022 23:11:23 +0000
>>> Subject: [PATCH] Fix reverse stepping multiple contiguous PC ranges
>>>
>>> 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 376a541faf6..997042d3e45 100644
>>> --- a/gdb/infrun.c
>>> +++ b/gdb/infrun.c
>>> @@ -6782,11 +6782,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 fi> +
>>> +
>>> + 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
>>
>> I think this could be moved to the line above.
>>
>
> Do you mean moving the range_start check downwards below the ecs->stop_func_start check?
I meant that they can be in the same line:
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 1a39372aad0..c40739919d1 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;
>>
>> Shouldn't prev_sal.line also be checked here and return an empty optional? I am not sure when that happens, so please enlighten me if there is no need to check.
>>
>
> I went through this again, and I don't think prev-sal.line needs to be checked. At this point we know current_sal.line is sane, so anything that differs from the current line, we bail out and return the address we currently have.
>
> Does that make sense?
>
It does. My main point was asking if finding a 0 was cause for leaving with an error, instead of leaving with the answer that could be wrong. But the base answer is already wrong, so this would probably be a less wrong answer, so this is fine
--
Cheers!
Bruno Larsen
next prev parent reply other threads:[~2022-03-10 13:20 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-23 16:39 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 [this message]
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
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=49264d6f-d66c-774e-f57b-9eb2efd767d5@redhat.com \
--to=blarsen@redhat.com \
--cc=cel@us.ibm.com \
--cc=gdb-patches@sourceware.org \
--cc=luis.machado@arm.com \
--cc=nd@arm.com \
--cc=rogealve@br.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).