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

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

Hi Carl,

Sorry for the e-mail bounce.

I can revisit this one, as I still see the same failures. Hopefully I'll 
have some time to address some of the concerns from past reviews. I'll 
need to refresh my memory on what it was about. :-)

Regards,
Luis

On 3/8/22 22:01, Carl Love via Gdb-patches wrote:
> On Tue, 2022-03-08 at 17:21 -0300, Bruno Larsen wrote:
> 
> <snip>
>>
>>
>> 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
>>
> 
> I have not writen anything in dwarf assembler in the past.  Off the top
> of my head, don't know how to create such a test case.  It does seem
> that there are the two testcases  gdb.reverse/solib-precsave.exp and
> gdb.reverse/solib-reverse.exp which the patch fixes.  I guess the dwarf
> assembly test would be similar to the C level code.
> 
> Do you have an example of how to write dwarf assembly or a pointer to a
> tutorial on writting dwarf?
> 
> I will work on the two comments you had on the patch.  Thanks for your
> time reviewing the patch.
> 
>                      Carl
> 
> 
>>> -----------------------------------------------------------
>>> 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.
>>
>>>    	  && 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.
>>
>>> +    }
>>> +
>>> +  return prev_pc;
>>> +}
>>> +
>>>    /* See symtab.h.  */
>>>    
>>>    struct symtab *
>>> diff --git a/gdb/symtab.h b/gdb/symtab.h
>>> index d12eee6e9d8..4d893a8a3b8 100644
>>> --- a/gdb/symtab.h
>>> +++ b/gdb/symtab.h
>>> @@ -2149,6 +2149,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);
>>
>>
>