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

Message ID 670af48c5b21df5942cc7c806b949b78e41fcd56.camel@us.ibm.com
State New
Headers show
Series
  • finish-reverse fix for setting break on correct entry point when executing in reverse.
Related show

Commit Message

Simon Marchi via Gdb-patches July 9, 2021, 9:22 p.m.
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(+)

-- 
2.17.1

Comments

Simon Marchi via Gdb-patches July 21, 2021, 4:39 p.m. | #1
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,
Simon Marchi via Gdb-patches July 24, 2021, 3:32 p.m. | #2
"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
Simon Marchi via Gdb-patches July 29, 2021, 9 p.m. | #3
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

Patch

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,