From: Luis Machado <luis.machado@linaro.org>
To: gdb-patches@sourceware.org
Subject: [PING][PATCH] Fix reverse stepping multiple contiguous PC ranges
Date: Wed, 7 Apr 2021 12:23:20 -0300 [thread overview]
Message-ID: <966b7aa7-d5a5-a52a-1743-269ebd452f55@linaro.org> (raw)
In-Reply-To: <4677f871-070e-def5-9439-61310b177611@linaro.org>
Ping!
On 3/19/21 3:17 PM, Luis Machado wrote:
> Polite ping.
>
> On 3/12/21 12:36 PM, Luis Machado wrote:
>> Polite ping.
>>
>> On 3/4/21 11:30 AM, Luis Machado wrote:
>>> On 2/26/21 3:58 PM, Luis Machado wrote:
>>>> On 2/11/21 8:38 AM, Luis Machado wrote:
>>>>> On 2/1/21 4:19 PM, Luis Machado 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.
>>>>>>
>>>>>> 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 <luis.machado@linaro.org>
>>>>>>
>>>>>> * 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<CORE_ADDR> real_range_start
>>>>>> + = find_line_range_start
>>>>>> (ecs->event_thread->suspend.stop_pc);
>>>>>> +
>>>>>> + 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->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<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 f060e0ebc1..659cb46292 100644
>>>>>> --- a/gdb/symtab.h
>>>>>> +++ b/gdb/symtab.h
>>>>>> @@ -1916,6 +1916,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);
>>>>>>
next prev parent reply other threads:[~2021-04-07 15:23 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-01 19:19 [PATCH 3/3] " Luis Machado
2021-02-11 11:38 ` [PING][PATCH] " Luis Machado
2021-02-26 18:58 ` Luis Machado
2021-03-04 14:30 ` Luis Machado
2021-03-12 15:36 ` Luis Machado
2021-03-19 18:17 ` Luis Machado
2021-04-07 15:23 ` Luis Machado [this message]
2021-06-10 11:49 ` Luis Machado
2021-07-09 17:54 ` Carl Love
2021-07-01 13:54 ` Luis Machado
2021-07-13 2:53 ` [PATCH 3/3] " Simon Marchi
2021-07-20 15:06 ` 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=966b7aa7-d5a5-a52a-1743-269ebd452f55@linaro.org \
--to=luis.machado@linaro.org \
--cc=gdb-patches@sourceware.org \
/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).