Set the stepping range from the function at PC, not at stop_pc.

Set the stepping range from the function at PC, not at stop_pc.

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 1833 bytes --]

Following up on:
 http://sourceware.org/ml/gdb-patches/2008-12/msg00100.html

Here goes.  This addresses this valid use case:

< stopped at 0x1234, thread 1, (stop_pc == 0x1234) >
 (gdb) set $pc = 0xf00
 (gdb) step

GDB at this point would set the stepping range based on the
function at 0x1234, but execution was about to resume at 0xf00.

GDB should instead be setting the stepping range from the
function at 0xf00.

I think it's obvious after the discussion, but I'd still like to
have your approval.  OK?  Any objections?

On Friday 05 December 2008 19:06:14, Pedro Alves wrote:
> On Friday 05 December 2008 18:42:45, Ulrich Weigand wrote:
> > Pedro Alves wrote:
> > 
> > > > > > <stopped at 0x1234, thread 1>
> > > > > >  (gdb) set $pc = 0xf00
> > > > > >  (gdb) call func()
> > > > > 
> > > > > Huh.  But that case is in fact *broken*, because GDB will use stop_pc
> > > > > incorrectly: for example, the check whether we are about to continue
> > > > > at a breakpoint will look at stop_pc, but then continue at $pc.  
> > > > 
> > > > This one I believe was the original intention.  The rationale being
> > > > that you'd not want to hit a breakpoint again at stop_pc (0x1234),
> > > > because there's where you stopped; but, you'd want to hit a a breakpoint
> > > > at 0xf00, sort of like jump *$pc hits a breakpoint at $pc.
> > > > 
> > > > Note, I'm not saying I agree with this.  I did say that probably nobody
> > > > would notice if we got rid of stop_pc.
> > 
> > OK, I see.  This is a valid use case, and it may make sense to keep it.
> > However, as you point out, to make this really work as intended, we'd
> > have make stop_pc a per-thread variable.
> > 
> > And even in that case, the uses of stop_pc in step_1 and step_once seem
> > invalid to me.
> > 
> 
> 100% Agreed.  I'll take care of it.

-- 
Pedro Alves

[-- Attachment #2: step_func_pc.diff --]
[-- Type: text/x-diff, Size: 1678 bytes --]

2008-12-05  Pedro Alves  <pedro@codesourcery.com>

	* infcmd.c (step_1, step_once): Look up the stepping range based
	on the current PC, not on stop_pc.

---
 gdb/infcmd.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Index: src/gdb/infcmd.c
===================================================================
--- src.orig/gdb/infcmd.c	2008-12-05 19:20:20.000000000 +0000
+++ src/gdb/infcmd.c	2008-12-05 19:27:49.000000000 +0000
@@ -812,12 +812,15 @@ step_1 (int skip_subroutines, int single
 
 	  if (!single_inst)
 	    {
-	      find_pc_line_pc_range (stop_pc,
+	      CORE_ADDR pc;
+
+	      pc = read_pc ();
+	      find_pc_line_pc_range (pc,
 				     &tp->step_range_start, &tp->step_range_end);
 	      if (tp->step_range_end == 0)
 		{
 		  char *name;
-		  if (find_pc_partial_function (stop_pc, &name,
+		  if (find_pc_partial_function (pc, &name,
 						&tp->step_range_start,
 						&tp->step_range_end) == 0)
 		    error (_("Cannot find bounds of current function"));
@@ -932,7 +935,10 @@ step_once (int skip_subroutines, int sin
 
       if (!single_inst)
 	{
-	  find_pc_line_pc_range (stop_pc,
+	  CORE_ADDR pc;
+
+	  pc = read_pc ();
+	  find_pc_line_pc_range (pc,
 				 &tp->step_range_start, &tp->step_range_end);
 
 	  /* If we have no line info, switch to stepi mode.  */
@@ -943,7 +949,7 @@ step_once (int skip_subroutines, int sin
 	  else if (tp->step_range_end == 0)
 	    {
 	      char *name;
-	      if (find_pc_partial_function (stop_pc, &name,
+	      if (find_pc_partial_function (pc, &name,
 					    &tp->step_range_start,
 					    &tp->step_range_end) == 0)
 		error (_("Cannot find bounds of current function"));

Re: Set the stepping range from the function@PC, not@stop_pc.

[Ulrich Weigand]

Pedro Alves wrote:

> Following up on:
>  http://sourceware.org/ml/gdb-patches/2008-12/msg00100.html
> 
> Here goes.  This addresses this valid use case:
> 
> < stopped at 0x1234, thread 1, (stop_pc == 0x1234) >
>  (gdb) set $pc = 0xf00
>  (gdb) step
> 
> GDB at this point would set the stepping range based on the
> function at 0x1234, but execution was about to resume at 0xf00.
> 
> GDB should instead be setting the stepping range from the
> function at 0xf00.

Thanks!

> I think it's obvious after the discussion, but I'd still like to
> have your approval.  OK?  Any objections?

It might be better to use get_frame_pc (frame) instead of read_pc (),
as we already set up the step_frame_id relative to that frame.  But
this is just a minor preference ...

Otherwise, this looks good to me.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: Set the stepping range from the function@PC, not@stop_pc.

[Daniel Jacobowitz]

On Fri, Dec 05, 2008 at 09:30:38PM +0100, Ulrich Weigand wrote:
> > I think it's obvious after the discussion, but I'd still like to
> > have your approval.  OK?  Any objections?
> 
> It might be better to use get_frame_pc (frame) instead of read_pc (),

Yes, please.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Set the stepping range from the function@PC, not@stop_pc.

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 557 bytes --]

On Friday 05 December 2008 20:43:28, Daniel Jacobowitz wrote:
> On Fri, Dec 05, 2008 at 09:30:38PM +0100, Ulrich Weigand wrote:
> > > I think it's obvious after the discussion, but I'd still like to
> > > have your approval.  OK?  Any objections?
> > 
> > It might be better to use get_frame_pc (frame) instead of read_pc (),
> 
> Yes, please.
> 

Good idea.  I wonder if this I've checked this in then.  There are two
other instances of read_pc in infcmd.c that could use the same idiom --- I
guess these calls predate the sentinel frame.

-- 
Pedro Alves

[-- Attachment #2: step_func_pc.diff --]
[-- Type: text/x-diff, Size: 1706 bytes --]

2008-12-05  Pedro Alves  <pedro@codesourcery.com>

	* infcmd.c (step_1, step_once): Look up the stepping range based
	on the current frame's PC, not on stop_pc.

---
 gdb/infcmd.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Index: src/gdb/infcmd.c
===================================================================
--- src.orig/gdb/infcmd.c	2008-12-05 20:03:59.000000000 +0000
+++ src/gdb/infcmd.c	2008-12-05 21:42:56.000000000 +0000
@@ -812,12 +812,15 @@ step_1 (int skip_subroutines, int single
 
 	  if (!single_inst)
 	    {
-	      find_pc_line_pc_range (stop_pc,
+	      CORE_ADDR pc;
+
+	      pc = get_frame_pc (frame);
+	      find_pc_line_pc_range (pc,
 				     &tp->step_range_start, &tp->step_range_end);
 	      if (tp->step_range_end == 0)
 		{
 		  char *name;
-		  if (find_pc_partial_function (stop_pc, &name,
+		  if (find_pc_partial_function (pc, &name,
 						&tp->step_range_start,
 						&tp->step_range_end) == 0)
 		    error (_("Cannot find bounds of current function"));
@@ -932,7 +935,10 @@ step_once (int skip_subroutines, int sin
 
       if (!single_inst)
 	{
-	  find_pc_line_pc_range (stop_pc,
+	  CORE_ADDR pc;
+
+	  pc = get_frame_pc (frame);
+	  find_pc_line_pc_range (pc,
 				 &tp->step_range_start, &tp->step_range_end);
 
 	  /* If we have no line info, switch to stepi mode.  */
@@ -943,7 +949,7 @@ step_once (int skip_subroutines, int sin
 	  else if (tp->step_range_end == 0)
 	    {
 	      char *name;
-	      if (find_pc_partial_function (stop_pc, &name,
+	      if (find_pc_partial_function (pc, &name,
 					    &tp->step_range_start,
 					    &tp->step_range_end) == 0)
 		error (_("Cannot find bounds of current function"));

Re: Set the stepping range from the function@PC, not@stop_pc.

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 344 bytes --]

On Friday 05 December 2008 22:43:13, Pedro Alves wrote:

> Good idea.  I wonder if this I've checked this in then.  There are two
> other instances of read_pc in infcmd.c that could use the same idiom --- I
> guess these calls predate the sentinel frame.

Might as well be consistent.  Tested on x86-64-unknown-linux-gnu.

OK?

-- 
Pedro Alves

[-- Attachment #2: get_frame_pc.diff --]
[-- Type: text/x-diff, Size: 1513 bytes --]

2008-12-05  Pedro Alves  <pedro@codesourcery.com>

	* infcmd.c (until_next_command, finish_backward): Use get_frame_pc
	instead of read_pc.

---
 gdb/infcmd.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Index: src/gdb/infcmd.c
===================================================================
--- src.orig/gdb/infcmd.c	2008-12-05 23:02:12.000000000 +0000
+++ src/gdb/infcmd.c	2008-12-05 23:19:21.000000000 +0000
@@ -1181,7 +1181,7 @@ until_next_command (int from_tty)
      than the current line (if in symbolic section) or pc (if
      not). */
 
-  pc = read_pc ();
+  pc = get_frame_pc (frame);
   func = find_pc_function (pc);
 
   if (!func)
@@ -1405,11 +1405,13 @@ finish_backward (struct symbol *function
   struct thread_info *tp = inferior_thread ();
   struct breakpoint *breakpoint;
   struct cleanup *old_chain;
+  CORE_ADDR pc;
   CORE_ADDR func_addr;
   int back_up;
 
-  if (find_pc_partial_function (get_frame_pc (get_current_frame ()),
-				NULL, &func_addr, NULL) == 0)
+  pc = get_frame_pc (get_current_frame ());
+
+  if (find_pc_partial_function (pc, NULL, &func_addr, NULL) == 0)
     internal_error (__FILE__, __LINE__,
 		    _("Finish: couldn't find function."));
 
@@ -1426,7 +1428,7 @@ finish_backward (struct symbol *function
      no way that a function up the stack can have a return address
      that's equal to its entry point.  */
 
-  if (sal.pc != read_pc ())
+  if (sal.pc != pc)
     {
       /* Set breakpoint and continue.  */
       breakpoint =

Re: Set the stepping range from the function@PC, not@stop_pc.

[Pedro Alves]

On Friday 05 December 2008 23:36:52, Pedro Alves wrote:
> 2008-12-05  Pedro Alves  <pedro@codesourcery.com>
> 
>         * infcmd.c (until_next_command, finish_backward): Use get_frame_pc
>         instead of read_pc.
> 

Guys, I'm checking this in.  Let me know if this turns up any issue.

-- 
Pedro Alves