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

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

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?

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