[PATCH] finish-reverse fix for setting break on correct entry point when executing in reverse.

[PATCH] finish-reverse fix for setting break on correct entry point when executing in reverse.

[Carl Love]

GDB maintainers:

The following patch fixes issues with reverse execution on PPC64. 
PPC64 has the concept of local and remote entry points to a function. 
Currently the reverse execution sets a break point on the gloabal
function entry point even if the forward execution entered via the
local entry point.  The issue is that when executing a reverse step,
the breakpoint on the function is not seen since reverse execution
follows the local entry point.  Thus the finish command doesn't stop at
the begining of the function but rather at the next breakpoint
encountered.  

This patch corrects the problem by calling the function
gdbarch_skip_entrypoint to update the sal.pc if needed so the
breakpoint that gets set is for the local entry point.

The patch has been tested on Power 9, Power 10 and X86 64bit with no
regressions.  

The patch fixes 11 failures on PPC64 for test gdb.reverse/finish-
reverse.exp and 11 failures for test gdb.reverse/finish-precsave.exp.

Please let me know if the patch is acceptable for committing to
mainline.   Thanks.

                 Carl 


----------------------------------------------------------


PPC64 has both global and local entry points.  When executing in reverse
the function finish_backward sets the break point at the global entry
point.  However if the forward execution enters via the local entry point
reverse execution never sees the break point at the entry of the function.
Reverse execution continues until the next break point is encountered thus
stopping at the wrong place.

This patch adds a check and a call to gdbarch_skip_entrypoint to fix up
the sal.pc value to the local entry point if appropriate.  The patch fixes
eleven regression test failures in gdb.reverse/finish-reverse.exp for PPC64.

gdb/ChangeLog
2021-07-09  Carl Love  <cel@us.ibm.com>
	* infcmd.c (finish_backward): Add if gdbarch_skip_entrypoint_p and
	call to gdbarch_skip_entrypoint.
---
 gdb/infcmd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 8190ba36565..cce244b08a0 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1707,6 +1707,12 @@ finish_backward (struct finish_command_fsm *sm)
       /* Set a step-resume at the function's entry point.  Once that's
 	 hit, we'll do one more step backwards.  */
       symtab_and_line sr_sal;
+
+      /* Some architectures, like PPC64 uses local and global entry points.
+	 May need to adjust sal.pc if the local entry point was used.  */
+      if (gdbarch_skip_entrypoint_p (gdbarch))
+        sal.pc = gdbarch_skip_entrypoint (gdbarch, sal.pc);
+
       sr_sal.pc = sal.pc;
       sr_sal.pspace = get_frame_program_space (frame);
       insert_step_resume_breakpoint_at_sal (gdbarch,
-- 
2.17.1

Re: [PATCH] finish-reverse fix for setting break on correct entry point when executing in reverse.

[Carl Love]

GDB maintainers:

Ping, I have not seen any feedback on this patch.  If someone could
take a look at it and let me know what they think I would appreciate
it.  Thanks.

                 Carl 
--------------------------------------

On Fri, 2021-07-09 at 14:22 -0700, Carl Love wrote:
> GDB maintainers:
> 
> The following patch fixes issues with reverse execution on PPC64. 
> PPC64 has the concept of local and remote entry points to a
> function. 
> Currently the reverse execution sets a break point on the gloabal
> function entry point even if the forward execution entered via the
> local entry point.  The issue is that when executing a reverse step,
> the breakpoint on the function is not seen since reverse execution
> follows the local entry point.  Thus the finish command doesn't stop
> at
> the begining of the function but rather at the next breakpoint
> encountered.  
> 
> This patch corrects the problem by calling the function
> gdbarch_skip_entrypoint to update the sal.pc if needed so the
> breakpoint that gets set is for the local entry point.
> 
> The patch has been tested on Power 9, Power 10 and X86 64bit with no
> regressions.  
> 
> The patch fixes 11 failures on PPC64 for test gdb.reverse/finish-
> reverse.exp and 11 failures for test gdb.reverse/finish-precsave.exp.
> 
> Please let me know if the patch is acceptable for committing to
> mainline.   Thanks.
> 
>                  Carl 
> 
> 
> ----------------------------------------------------------
> 
> 
> PPC64 has both global and local entry points.  When executing in
> reverse
> the function finish_backward sets the break point at the global entry
> point.  However if the forward execution enters via the local entry
> point
> reverse execution never sees the break point at the entry of the
> function.
> Reverse execution continues until the next break point is encountered
> thus
> stopping at the wrong place.
> 
> This patch adds a check and a call to gdbarch_skip_entrypoint to fix
> up
> the sal.pc value to the local entry point if appropriate.  The patch
> fixes
> eleven regression test failures in gdb.reverse/finish-reverse.exp for
> PPC64.
> 
> gdb/ChangeLog
> 2021-07-09  Carl Love  <cel@us.ibm.com>
> 	* infcmd.c (finish_backward): Add if gdbarch_skip_entrypoint_p
> and
> 	call to gdbarch_skip_entrypoint.
> ---
>  gdb/infcmd.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 8190ba36565..cce244b08a0 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1707,6 +1707,12 @@ finish_backward (struct finish_command_fsm
> *sm)
>        /* Set a step-resume at the function's entry point.  Once
> that's
>  	 hit, we'll do one more step backwards.  */
>        symtab_and_line sr_sal;
> +
> +      /* Some architectures, like PPC64 uses local and global entry
> points.
> +	 May need to adjust sal.pc if the local entry point was
> used.  */
> +      if (gdbarch_skip_entrypoint_p (gdbarch))
> +        sal.pc = gdbarch_skip_entrypoint (gdbarch, sal.pc);
> +
>        sr_sal.pc = sal.pc;
>        sr_sal.pspace = get_frame_program_space (frame);
>        insert_step_resume_breakpoint_at_sal (gdbarch,

Re: [PATCH] finish-reverse fix for setting break on correct entry point when executing in reverse.

[Ulrich Weigand]

"Carl Love" <cel@us.ibm.com> wrote on 21.07.2021 18:39:16:

> gdb/ChangeLog
> 2021-07-09  Carl Love  <cel@us.ibm.com>
>    * infcmd.c (finish_backward): Add if gdbarch_skip_entrypoint_p and
>    call to gdbarch_skip_entrypoint.

I think you correctly identified the problem, but the fix is not
quite correct.  Note that you may actually be on the global entry
point (e.g. if you did a reverse single-step before) - in that
case the code with your change would set a breakpoint on the
*local* entry point, but that will never be reached any more.

I think the correct fix in the case where we have global and local
entry points would to always reverse single-step whenever the
current PC falls between the two entry points (inclusively), and
reverse continue to the local entry point otherwise.

Bye,
Ulrich

Re: [PATCH] finish-reverse fix for setting break on correct entry point when executing in reverse.

[Carl Love]

Ulrich

On Sat, 2021-07-24 at 17:32 +0200, Ulrich Weigand wrote:
> "Carl Love" <cel@us.ibm.com> wrote on 21.07.2021 18:39:16:
> 
> > gdb/ChangeLog
> > 2021-07-09  Carl Love  <cel@us.ibm.com>
> >    * infcmd.c (finish_backward): Add if gdbarch_skip_entrypoint_p
> and
> >    call to gdbarch_skip_entrypoint.
> 
> I think you correctly identified the problem, but the fix is not
> quite correct.  Note that you may actually be on the global entry
> point (e.g. if you did a reverse single-step before) - in that
> case the code with your change would set a breakpoint on the
> *local* entry point, but that will never be reached any more.
> 
> I think the correct fix in the case where we have global and local
> entry points would to always reverse single-step whenever the
> current PC falls between the two entry points (inclusively), and
> reverse continue to the local entry point otherwise.

I reworked the patach per our discussion.  I retested the patch on
Intel, Power 9 and Power 10 without any regressions.  

I put debug prints in to see when I hit the case of the PC being past
the local and global entry points as well as the case of the PC being
between the local and global entry points.  When I run the entire
regression suite, I did not see a case where the PC was between the two
entry points.  So unfortunately, that code is untested.  I have not
come up with a test for that case.

Please let me know if the patch looks ok and if you know of a case that
could be used to test the case of the PC being between the local and
global entry points.  Thanks.

                   Carl 


----------------------------------------------------------------------

finish-reverse fix for setting break on correct entry point when executing in reverse.

PPC64 has both global and local entry points.  When executing in reverse
the function finish_backward sets the break point at the global entry
point.  However if the forward execution enters via the local entry point
reverse execution never sees the break point at the global entry point of the
function.  Reverse execution continues until the next break point is encountered
thus stopping at the wrong place.

This patch adds a check to backup to the local breakpoint if we are in the
body of the function.  GDB will than do an additional step backwards.  If
the function was entered via the local entry point, execution will now be
stopped at the call to the function.  If the function was entered via the
global entry point, the pc will be between the local and global entry
points.  In that case, execution backs up to the global entry point.

gdb/ChangeLog
2021-07-27  Carl Love  <cel@us.ibm.com>
	* infcmd.c (finish_backward): Add if gdbarch_skip_entrypoint_p and
	call to gdbarch_skip_entrypoint.
---
 gdb/infcmd.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 0a5edef6982..4834d71ebe8 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1708,11 +1708,36 @@ finish_backward (struct finish_command_fsm *sm)
       /* Set a step-resume at the function's entry point.  Once that's
 	 hit, we'll do one more step backwards.  */
       symtab_and_line sr_sal;
+
+      /* Some architectures, like PPC64 uses local and global entry points.
+	 May need to adjust sal.pc if the local entry point was used.  */
+      if (gdbarch_skip_entrypoint_p (gdbarch)) {
+	CORE_ADDR local_entry_point
+	  = gdbarch_skip_entrypoint (gdbarch, sal.pc);
+
+	/* If we are in the "body" of the function, back up to the local
+	   entry point.  Either we will end up in the caller if the local
+	   entry point was the entry point.  Or we return to this function
+	   with the pc between the local and global entry points.  In that
+	   case we backup to the global entry point.  */
+	if  ((pc > sal.pc)  &&     // in body of function
+	       (pc > local_entry_point)) {
+	  sal.pc = local_entry_point;
+	  tp->control.step_range_start = pc;
+	  tp->control.step_range_end = local_entry_point;
+
+	} else 	if  ((pc > sal.pc)  &&     // between entry points
+	       (pc < local_entry_point)) {
+	  /* Back up to the global entry point.  */
+	  tp->control.step_range_start = pc;
+	  tp->control.step_range_end = sal.pc;
+	}
+      }
+
       sr_sal.pc = sal.pc;
       sr_sal.pspace = get_frame_program_space (frame);
       insert_step_resume_breakpoint_at_sal (gdbarch,
 					    sr_sal, null_frame_id);
-
       proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
     }
   else
-- 
2.17.1

Re: [PATCH] finish-reverse fix for setting break on correct entry point when executing in reverse.

[Carl Love]

Ulrich:

> I put debug prints in to see when I hit the case of the PC being past
> the local and global entry points as well as the case of the PC being
> between the local and global entry points.  When I run the entire
> regression suite, I did not see a case where the PC was between the
> two
> entry points.  So unfortunately, that code is untested.  I have not
> come up with a test for that case.
> 

I have been playing with the patch with help from Will.  After several
attempts to create a test case that will use the global entry point, we
found that creating a .so library function, Will's idea, creates the
needed test case.  Unfortunately, I can't see the C code in the library
function I can use ni, si to move in the function and disas to see
where in the code I am.  

I have debug prints in the code so I can see which execution path I am
in.  The code from the patch with the debug prints is as follows:

                                                                               
      /* Some architectures, like PPC64 uses local and global entry points.     
         May need to adjust sal.pc if the local entry point was used.  */       
      if (gdbarch_skip_entrypoint_p (gdbarch)) {                                
        CORE_ADDR local_entry_point                                             
          = gdbarch_skip_entrypoint (gdbarch, sal.pc);                          
                                                                                
        /* If we are in the "body" of the function, back up to the local        
           entry point.  Either we will end up in the caller if the local       
           entry point was the entry point.  Or we return to this function      
           with the pc between the local and global entry points.  In that      
           case we backup to the global entry point.  */                        
        if  ((pc > sal.pc)  &&     // in body of function                       
               (pc > local_entry_point)) {                                      
          printf_filtered ("Carll go back to local entry point\n");             
          sal.pc = local_entry_point;                                           
          tp->control.step_range_start = pc;                                    
          tp->control.step_range_end = local_entry_point;                       
                                                                                
        } else  if  ((pc > sal.pc)  &&     // between entry points              
               (pc < local_entry_point)) {                                      
          /* Back up to the global entry point.  */                             
          printf_filtered ("Carll go back to global entry point\n");            
          tp->control.step_range_start = pc;                                    
          tp->control.step_range_end = sal.pc;                                  
        }                                                                       
      }  

In gdb, I start with "break main" followed by "run".  Then "record" so
I can do the reverse execution.  I than do "break get_number_rand"
where get_number_rand is the function I wrote and is in the .so
library.  Now "continue" and I get to:

(gdb) disas
Dump of assembler code for function get_number_rand:
   0x00007ffff7f5068c <+0>:	addis   r2,r12,2
   0x00007ffff7f50690 <+4>:	addi    r2,r2,30836
   0x00007ffff7f50694 <+8>:	mflr    r0
   0x00007ffff7f50698 <+12>:	std     r0,16(r1)
   0x00007ffff7f5069c <+16>:	std     r31,-8(r1)
   0x00007ffff7f506a0 <+20>:	stdu    r1,-64(r1)
   0x00007ffff7f506a4 <+24>:	mr      r31,r1
=> 0x00007ffff7f506a8 <+28>:	bl      0x7ffff7f50500 <0000001a.plt_call.rand@@GLIBC_2.17>
   0x00007ffff7f506ac <+32>:	ld      r2,24(r1)
   0x00007ffff7f506b0 <+36>:	mr      r9,r3
   0x00007ffff7f506b4 <+40>:	stw     r9,36(r31)
   0x00007ffff7f506b8 <+44>:	bl      0x7ffff7f50500 <0000001a.plt_call.rand

I did a couple of "ni" commands to move into the body of the function
as the original test case did.  The pc is now at 0x00007ffff7f506b0. 
Now I issue the command " set exec-dir reverse".  The next command is
"continue" as was done ith the original test.  The PC is now back at
0x00007ffff7f506a8 <+28>.  Now the "finish" command is issued and the
disas command shows the pc is at 0x00007ffff7f50690 and I see the debug
print "go back to local entry point".  I am actually supposed to be at
the call in the main routine.  I tried issuing the "finish" command
again, justs to see what would happen.  I see the debug statement "go
back to global entry point".  The PC is now back at the call to the
get_number_rand in the main program as expected.

=> 0x00000001000009b0 <+116>:	bl      0x100000760 <0000001b.plt_call.get_number_rand>

So, I was able to exercise the code but it actually took two finish
commands to get to where I was supposed to be.  So, this doesn't really
work as desired for the global entry case.

Seems like we need to remember if we are doing a finish command and
than check when we stop if we are still in the function.  I will have
to look at it more to see if I can find a way to make that work.  

                        Carl 

                                         

Re: [PATCH] finish-reverse fix for setting break on correct entry point when executing in reverse.

[Ulrich Weigand]

"Carl Love" <cel@us.ibm.com> wrote on 05.08.2021 01:51:56:

> So, I was able to exercise the code but it actually took two finish
> commands to get to where I was supposed to be.  So, this doesn't really
> work as desired for the global entry case.
>
> Seems like we need to remember if we are doing a finish command and
> than check when we stop if we are still in the function.

It seems to me this is exactly what I was mentioning in my email
as of 07/27, this has to be handled in the infrun state machine
in process_event_stop_test.   Quoting from that email:

>If you need to use the step-resume breakpoint, you'll still need to
>step after that breakpoint is hit.  This is currently being done in
>the state machine handling in process_event_stop_test, case
>BPSTAT_WHAT_STEP_RESUME:
>     if (ecs->event_thread->control.proceed_to_finish
>         && execution_direction == EXEC_REVERSE)
>       {
>         struct thread_info *tp = ecs->event_thread;
>
>         /* We are finishing a function in reverse, and just hit the
>            step-resume breakpoint at the start address of the
>            function, and we're almost there -- just need to back up
>            by one more single-step, which should take us back to the
>            function call.  */
>         tp->control.step_range_start = tp->control.step_range_end = 1;
>         keep_going (ecs);
>         return;
>       }
>
>
>Again, the only required change here is to set up the step range
>properly.

Did you implement this change?

Bye,
Ulrich