Re: [PATCH] Updated, fix reverse stepping multiple contiguous PC ranges

[Luis Machado <luis.machado@arm.com>]

On 3/10/22 13:19, Bruno Larsen wrote:
> 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
> 

Got it. I can do that.

>>
>>>>         && 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
> 

Finding a line 0 could be either valid or invalid. If the line table is 
sane and we find 0 because there is no line number for a particular PC, 
it is valid, and we've reached the end of the lookup.

If we find 0 because the line table is missing an entry for the PC we're 
looking for, then the answer is incorrect. But that would be a problem 
with the line table. Not much GDB can do other than warn.

I'll refresh/re-test this patch and will address some previous comments 
Simon had.