[PING][PATCH] Fix reverse stepping multiple contiguous PC ranges

[Luis Machado <luis.machado@linaro.org>]

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);
>>>>>