Re: [PATCH 2/2 ver 7] Fix reverse stepping multiple contiguous PC ranges over the line table.

[Guinevere Larsen <blarsen@redhat.com>]

On 07/08/2023 21:03, Carl Love wrote:
>
> Bruno, Simon, GDB maintainers:

Heads up, I go by Guinevere now, no longer Bruno :)

Other than that, I have a very minor nit inlined, but  you can take or 
leave that one, honestly.

Reviewed-By: Guinevere Larsen <blarsen@redhat.com>

>
> Version 7, addressed behavior of GDB when stepping backward thru a
> function call as mentioned by Bruno.  GDB would stop at the function
> call and then a reverse step/next would cause GDB to stop at the first
> instruction of the same line as the function call instead of stopping
> in the previous line.  The behavior was fixed and the various test
> programs were updated to remove one of the two reverse step or next
> instructions required to reach the previous line.
>
> Version 6, fixed various code style issues in the GDB source.  The
> testcases were updated to use with_test_prefix for each gdb test in the
> step and next test cases, switch using the standard_testfile, use
> foreach_with_prefix to combine otherwise identical tests.  Retested on
> Power 10.
>
> Version 5, changed comments in test case func-map-to-same-line.c.
> Patch 1/2 implemented the new options for gdb_compile.  Updated the
> call to proc run_tests to use the new gdb_compile options in a
> foreach_with_prefix loop.
>
> Version 4, additional fixes for gcc version check, wrap function calls
> using "with_test_prefix", move load_lib dwarf.exe. Fixed typo noted by
> Luis.
>
> Version 3, added the gcc version check as discussed further from
> version 2 of the patch.  Also updated the tests to check for supporting
> reverse execution rather than requiring recording.  I also noticed
> there were a couple more instances of a requirement check, i.e. if []
> which I changed to "require" per the current style for checking on the
> test requirements.
>
>
> The following patch fixes issues on PowerPC with the reverse-step and
> reverse-next instructions when there are multiple assignment statements
> on the same line and when there are multiple function calls on the same
> line. The commit log below discusses these issues in further depth.
> The discussion included what the correct operation should be for these
> commands based on the GDB documentation.  The proposed patch at that
> time changed how the commands worked on other platforms such as X86 in
> a way they no longer matched the documentation.
>
> The issue is the line table contains multiple entries for the same
> source line.  The patch adds a function to search the line table to
> find the address of the first instruction of a line.  When setup up the
> reverse stepping range, the function is called to make sure the start
> of the range corresponds to the address of the first instruction for
> the line.  This approach was used.  When Luis initially developed the
> patch, he considered merging the contiguous ranges in the line table
> when reading the line tables. He decided it was better to work with the
> data directly in the line table rather than creating and using a
> modified version of the line table.
>
> The following patch fixes the execution of the reveres-step and
> reverse-next commands for both senarios of multiple statements on the
> same line for PowerPC and aarch64-linux.  Unlike the previous patch, it
> does not change the operation of the commands on other platforms, i.e.
> X86.  The patch adds new test cases for both scenarios to verify they
> work correctly.
>
> The patch has been tested on PowerPC, Intel X86 and aarch64-linux with
> no new regression failures.
>
> Please let me know if the patch is acceptable for mainline.  Thanks.
>
>                     Carl
>
>
>
> --------------------------------
> Fix reverse stepping multiple contiguous PC ranges over the line table.
>
> There are a couple of scenarios where the GDB reverse-step and reverse-next
> commands do not work correctly.
>
> Scenario 1 issue description by Luis Machado:
>
> When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also spotted on
> the ppc backend), 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 38 will land on line 40.
> - step from line 40 will land on line 42.
>
> Reverse execution:
> - step from line 42 will land on line 40.
> - step from line 40 will land on line 40.
> - step from line 40 will land on line 38.
>
> V
> The problem here is that line 40 contains two contiguous but distinct
> PC ranges in the line table, like so:
>
> Line 40 - [0x7ec ~ 0x7f4]
> Line 40 - [0x7f4 ~ 0x7fc]
>
> The two distinct ranges are generated because GCC started outputting source
> column information, which GDB doesn't take into account at the moment.
>
> When stepping forward from line 40, we skip both of these ranges and land on
> line 42. When stepping backward from line 42, we stop at the start PC of the
> second (or first, going backwards) range of line 40.
>
> Since we've reached ecs->event_thread->control.step_range_start, we stop
> stepping backwards.
>
> ---------------------------------------------------------
>
> Scenario 2 issue described by Pedro Alves:
>
> The following explanation of the issue was taken from the gdb mailing list
> discussion of the withdrawn patch to change the behavior of the reverse-step
> and reverse-next commands.  Specifically, message from Pedro Alves
> <pedro@palves.net> where he demonstrates the issue where you have multiple
> function calls on the same source code line:
>
> https://sourceware.org/pipermail/gdb-patches/2023-January/196110.html
>
> The source line looks like:
>
>     func1 ();  func2 ();
>
> so stepping backwards over that line should always stop at the first
> instruction of the line, not in the middle.  Let's simplify this.
>
> Here's the full source code of my example:
>
> (gdb) list 1
> 1       void func1 ()
> 2       {
> 3       }
> 4
> 5       void func2 ()
> 6       {
> 7       }
> 8
> 9       int main ()
> 10      {
> 11        func1 (); func2 ();
> 12      }
>
> Compiled with:
>
>   $ gcc reverse.c -o reverse -g3 -O0
>   $ gcc -v
>   ...
>   gcc version 11.3.0 (Ubuntu 11.3.0-1ubuntu1~22.04)
>
> Now let's debug it with target record, using current gdb git master (f3d8ae90b236),
> without your patch:
>
>   $ gdb ~/reverse
>   GNU gdb (GDB) 14.0.50.20230124-git
>   ...
>   Reading symbols from /home/pedro/reverse...
>   (gdb) start
>   Temporary breakpoint 1 at 0x1147: file reverse.c, line 11.
>   Starting program: /home/pedro/reverse
>   [Thread debugging using libthread_db enabled]
>   Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>
>   Temporary breakpoint 1, main () at reverse.c:11
>   11        func1 (); func2 ();
>   (gdb) record
>
>   (gdb) disassemble /s
>   Dump of assembler code for function main:
>   reverse.c:
>   10      {
>      0x000055555555513f <+0>:     endbr64
>      0x0000555555555143 <+4>:     push   %rbp
>      0x0000555555555144 <+5>:     mov    %rsp,%rbp
>
>   11        func1 (); func2 ();
>   => 0x0000555555555147 <+8>:     mov    $0x0,%eax
>      0x000055555555514c <+13>:    call   0x555555555129 <func1>
>      0x0000555555555151 <+18>:    mov    $0x0,%eax
>      0x0000555555555156 <+23>:    call   0x555555555134 <func2>
>      0x000055555555515b <+28>:    mov    $0x0,%eax
>
>   12      }
>      0x0000555555555160 <+33>:    pop    %rbp
>      0x0000555555555161 <+34>:    ret
>   End of assembler dump.
>
>   (gdb) n
>   12      }
>
> So far so good, a "next" stepped over the whole of line 11 and stopped at line 12.
>
> Let's confirm where we are now:
>
>   (gdb) disassemble /s
>   Dump of assembler code for function main:
>   reverse.c:
>   10      {
>      0x000055555555513f <+0>:     endbr64
>      0x0000555555555143 <+4>:     push   %rbp
>      0x0000555555555144 <+5>:     mov    %rsp,%rbp
>
>   11        func1 (); func2 ();
>      0x0000555555555147 <+8>:     mov    $0x0,%eax
>      0x000055555555514c <+13>:    call   0x555555555129 <func1>
>      0x0000555555555151 <+18>:    mov    $0x0,%eax
>      0x0000555555555156 <+23>:    call   0x555555555134 <func2>
>      0x000055555555515b <+28>:    mov    $0x0,%eax
>
>   12      }
>   => 0x0000555555555160 <+33>:    pop    %rbp
>      0x0000555555555161 <+34>:    ret
>   End of assembler dump.
>
> Good, we're at the first instruction of line 12.
>
> Now let's undo the "next", with "reverse-next":
>
>   (gdb) reverse-next
>   11        func1 (); func2 ();
>
> Seemingly stopped at line 11.  Let's see exactly where:
>
>   (gdb) disassemble /s
>   Dump of assembler code for function main:
>   reverse.c:
>   10      {
>      0x000055555555513f <+0>:     endbr64
>      0x0000555555555143 <+4>:     push   %rbp
>      0x0000555555555144 <+5>:     mov    %rsp,%rbp
>
>   11        func1 (); func2 ();
>      0x0000555555555147 <+8>:     mov    $0x0,%eax
>      0x000055555555514c <+13>:    call   0x555555555129 <func1>
>   => 0x0000555555555151 <+18>:    mov    $0x0,%eax
>      0x0000555555555156 <+23>:    call   0x555555555134 <func2>
>      0x000055555555515b <+28>:    mov    $0x0,%eax
>
>   12      }
>      0x0000555555555160 <+33>:    pop    %rbp
>      0x0000555555555161 <+34>:    ret
>   End of assembler dump.
>   (gdb)
>
> And lo, we stopped in the middle of line 11!  That is a bug, we should have
> stepped back all the way to the beginning of the line.  The "reverse-next"
> should have fully undone the prior "next" command.
>
> The above issues were fixed by introducing a new function that looks 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.  The new start PC for the range
> is used for the control.step_range_start when setting up a step range.
>
> The test case gdb.reverse/map-to-same-line.exp is added to test the fix
> for the issues in scenario 1.
>
> The test case gdb.reverse/func-map-to-same-line.exp was added to test the
> fix for scenario 2 when the binary was compiled with and without line
> table information.
>
> bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28426
>
> Co-authored-by: Luis Machado <luis.machado@arm.com>
> Co-authored-by: Carl Love <cel@us.ibm.com>
> ---
>   gdb/infcmd.c                                  |  13 ++
>   gdb/infrun.c                                  |  59 +++++++
>   gdb/symtab.c                                  |  49 ++++++
>   gdb/symtab.h                                  |  16 ++
>   gdb/testsuite/gdb.mi/mi-reverse.exp           |   5 +-
>   .../gdb.reverse/finish-reverse-next.exp       |  42 ++---
>   .../gdb.reverse/func-map-to-same-line.c       |  37 +++++
>   .../gdb.reverse/func-map-to-same-line.exp     | 139 ++++++++++++++++
>   gdb/testsuite/gdb.reverse/map-to-same-line.c  |  58 +++++++
>   .../gdb.reverse/map-to-same-line.exp          | 153 ++++++++++++++++++
>   10 files changed, 537 insertions(+), 34 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-line.c
>   create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
>   create mode 100644 gdb/testsuite/gdb.reverse/map-to-same-line.c
>   create mode 100644 gdb/testsuite/gdb.reverse/map-to-same-line.exp
>
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 15702f84894..add0eadd8c1 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -982,6 +982,19 @@ prepare_one_step (thread_info *tp, struct step_command_fsm *sm)
>   				 &tp->control.step_range_start,
>   				 &tp->control.step_range_end);
>   
> +	  if (execution_direction == EXEC_REVERSE)
> +	    {
> +	      symtab_and_line sal = find_pc_line (pc, 0);
> +	      symtab_and_line sal_start
> +		= find_pc_line (tp->control.step_range_start, 0);
> +
> +	      if (sal.line == sal_start.line)
> +		/* Executing in reverse, the step_range_start address is in
> +		   the same line.  We want to stop in the previous line so
> +		   move step_range_start before the current line.  */
> +		tp->control.step_range_start--;
> +	    }
> +
>   	  /* There's a problem in gcc (PR gcc/98780) that causes missing line
>   	     table entries, which results in a too large stepping range.
>   	     Use inlined_subroutine info to make the range more narrow.  */
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 8286026e6c6..32ba852f227 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -115,6 +115,9 @@ static struct async_event_handler *infrun_async_inferior_event_token;
>      Starts off as -1, indicating "never enabled/disabled".  */
>   static int infrun_is_async = -1;
>   
> +static CORE_ADDR update_line_range_start (CORE_ADDR pc,
> +					  struct execution_control_state *ecs);
> +
>   /* See infrun.h.  */
>   
>   void
> @@ -6884,6 +6887,27 @@ handle_signal_stop (struct execution_control_state *ecs)
>     process_event_stop_test (ecs);
>   }
>   
> +/* Return the address for the beginning of the line.  */
> +
> +CORE_ADDR
> +update_line_range_start (CORE_ADDR pc, struct execution_control_state *ecs)
> +{
> +  /* The line table may have multiple entries for the same source code line.
> +     Given the PC, check the line table and return the PC that corresponds
> +     to the line table entry for the source line that PC is in.  */
> +  CORE_ADDR start_line_pc = ecs->event_thread->control.step_range_start;
> +  gdb::optional<CORE_ADDR> real_range_start;
> +
> +  /* Call find_line_range_start to get the smallest address in the
> +     linetable for multiple Line X entries in the line table.  */
> +  real_range_start = find_line_range_start (pc);
> +
> +  if (real_range_start.has_value ())
> +    start_line_pc = *real_range_start;
> +
> +  return start_line_pc;
> +}
> +
>   /* Come here when we've got some debug event / signal we can explain
>      (IOW, not a random signal), and test whether it should cause a
>      stop, or whether we should resume the inferior (transparently).
> @@ -7685,6 +7709,28 @@ process_event_stop_test (struct execution_control_state *ecs)
>   
>         if (stop_pc_sal.is_stmt)
>   	{
> +	  if (execution_direction == EXEC_REVERSE)
> +	    {
> +	      /* We are stepping backwards make sure we have reached the
> +		 beginning of the line.  */
> +	      CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> +	      CORE_ADDR start_line_pc
> +		= update_line_range_start (stop_pc, ecs);
> +
> +	      if (stop_pc != start_line_pc)
> +		{
> +		  /* Have not reached the beginning of the source code line.
> +		     Set a step range.  Execution should stop in any function
> +		     calls we execute back into before reaching the beginning
> +		     of the line.  */
> +		  ecs->event_thread->control.step_range_start = start_line_pc;
> +		  ecs->event_thread->control.step_range_end = stop_pc;
> +		  set_step_info (ecs->event_thread, frame, stop_pc_sal);
> +		  keep_going (ecs);
> +		  return;
> +		}
> +	    }
> +
>   	  /* We are at the start of a statement.
>   
>   	     So stop.  Note that we don't stop if we step into the middle of a
> @@ -7747,6 +7793,19 @@ process_event_stop_test (struct execution_control_state *ecs)
>       set_step_info (ecs->event_thread, frame, stop_pc_sal);
>   
>     infrun_debug_printf ("keep going");
> +
> +  if (execution_direction == EXEC_REVERSE)
> +    {
> +      CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> +
> +      /* Make sure the stop_pc is set to the beginning of the line.  */
> +      if (stop_pc != ecs->event_thread->control.step_range_start)
> +	{
> +	  stop_pc = update_line_range_start (stop_pc, ecs);
> +	  ecs->event_thread->control.step_range_start = stop_pc;
pretty small nit, but I think it is better to just use

ecs->event_thread->control.step_range_start
     = update_line_range_start (stop_pc, ecs);

Just because it is kind of weird, to see the step_range_start being set 
to stop pc, and made me do a double take :)

-- 
Cheers,
Guinevere Larsen
She/Her/Hers