[PATCH 3/3] Fix reverse stepping multiple contiguous PC ranges

[PATCH 3/3] Fix reverse stepping multiple contiguous PC ranges

[Luis Machado]

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

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

[Luis Machado]

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

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

[Luis Machado]

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

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

[Luis Machado]

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

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

[Luis Machado]

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

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

[Luis Machado]

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

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

[Luis Machado]

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

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

[Luis Machado]

Ping!

On 4/7/21 12:23 PM, Luis Machado wrote:
> 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);
>>>>>>>

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

[Luis Machado]

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

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

[Carl Love]

GDB maintainers:

I have applied this patch from Luis and tested it on PPC64.  It fixes
the issues I was investigating for test gdb.reverse/step-reverse.exp. 
The patch also fixes the PPC64 errors on the following tests:

gdb.reverse/step-precsave.exp
gdb.reverse/solib-reverse.exp
gdb.reverse/solib-precsave.exp

I have tested this patch on Power 9, Power 10 and X86 64-bit.  The
patch does not introduce any additional regressions on PPC64.

                 Carl Love

On Thu, 2021-06-10 at 08:49 -0300, Luis Machado wrote:
> Ping!
> On 4/7/21 12:23 PM, Luis Machado wrote:
> > 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);
> > > > > > > > 

Re: [PATCH 3/3] Fix reverse stepping multiple contiguous PC ranges

[Simon Marchi]

On 2021-02-01 2:19 p.m., Luis Machado via Gdb-patches 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.

I don't have a deep knowledge of this area, so I'd like to know, how
does it work for forward stepping over the example you gave above?  What
is the sequence of decisions that lead to GDB deciding to continue
stepping more?  Because I am thinking that what we do for reverse
stepping should be pretty much the same as what we do for forward
stepping, just the other way around.

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

Could it be that the new compiler adds column information?  That would
explain why it's now two ranges.

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

With what I just merged, you'll find that you need to replace
->suspend.stop_pc with `->stop_pc ()`.  Nothing major.

> +
> +	  if (real_range_start.has_value ())
> +	    range_start = *real_range_start;

The question I have while reading this is: what did set the original
range_start, ecs->event_thread->control.step_range_start?  Why didn't we
get the "real" range start at the time and have to fix it up now?

> +	}
> +
>        /* 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;

Above, should we compare more than just the line?

What I am thinking is the edge case where looking up PC would return an
SAL, say `a.c:40`, and looking up "PC - 1" would return `b.c:40`.  The
algorithm would think that we are still on the same line and keep
searching, when in fact it's a different line 40, so we should have
stopped.


Simon

Re: [PATCH 3/3] Fix reverse stepping multiple contiguous PC ranges

[Luis Machado]

cc-ing Carl.

On 7/12/21 11:53 PM, Simon Marchi wrote:
> On 2021-02-01 2:19 p.m., Luis Machado via Gdb-patches 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.
> 
> I don't have a deep knowledge of this area, so I'd like to know, how
> does it work for forward stepping over the example you gave above?  What
> is the sequence of decisions that lead to GDB deciding to continue
> stepping more?  Because I am thinking that what we do for reverse
> stepping should be pretty much the same as what we do for forward
> stepping, just the other way around.

I'll have to take a look at it again, sorry. It's been a while since I 
touched this and I only have a vague recollection of what the exact 
problem was.

But the short answer is "no", we don't handle forward execution in the 
exact same way as forward execution. There are corner cases.

I think it had something to do with how the line ranges are represented. 
The start address of a PC range is always contained in the range, but 
the ending address is not. When you move forwards/backwards, that makes 
a difference.

> 
>> 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.
> 
> Could it be that the new compiler adds column information?  That would
> explain why it's now two ranges.

Probably.

> 
>> 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);
> 
> With what I just merged, you'll find that you need to replace
> ->suspend.stop_pc with `->stop_pc ()`.  Nothing major.
> 
>> +
>> +	  if (real_range_start.has_value ())
>> +	    range_start = *real_range_start;
> 
> The question I have while reading this is: what did set the original
> range_start, ecs->event_thread->control.step_range_start?  Why didn't we
> get the "real" range start at the time and have to fix it up now?
> 

That's the bit I don't remember exactly. But GDB has no notion of lines 
with multiple PC ranges. It works by (from what I vaguely recall) 
skipping ranges based on the line number, start address and end address 
of a range. And in the case of backwards execution, it doesn't know how 
to figure out the line range is not over yet.

>> +	}
>> +
>>         /* 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;
> 
> Above, should we compare more than just the line?
> 
> What I am thinking is the edge case where looking up PC would return an
> SAL, say `a.c:40`, and looking up "PC - 1" would return `b.c:40`.  The
> algorithm would think that we are still on the same line and keep
> searching, when in fact it's a different line 40, so we should have
> stopped.

Yeah, that sounds worth covering in the code.

> 
> 
> Simon
>