Fix for PR 1971 .

Fix for PR 1971 .

[Ramana Radhakrishnan]

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

Hi , 

Attached is a fix for PR 1971. This inserts a breakpoint at the return
address for a function that does not have a previous frame which is what
you have in the case of main. This would however not stop after the
return from main because the semantics of the next command would not
stop the execution in any place where there is no debug information. 

Tested on native x86 with today's head as well as 6.4 branch with no
extra regressions .

Ok to commit with an additional testcase ?
 
cheers
Ramana

2006-01-03  Ramana Radhakrishnan  <ramana.radhakrishnan@codito.com>

	PR 1971
	* infrun.c (handle_inferior_event): Handle case where previous
	frame can be NULL. Use insert_step_resume_breakpoint_at_sal instead.


-- 
Ramana Radhakrishnan
GNU Tools
codito ergo sum (www.codito.com)

[-- Attachment #2: patch-pr1971 --]
[-- Type: text/x-patch, Size: 1187 bytes --]

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.208
diff -u -a -u -r1.208 infrun.c
--- infrun.c	17 Dec 2005 22:34:01 -0000	1.208
+++ infrun.c	3 Jan 2006 17:48:16 -0000
@@ -2390,8 +2390,22 @@
 	  /* We're doing a "next", set a breakpoint at callee's return
 	     address (the address at which the caller will
 	     resume).  */
-	  insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ()));
-	  keep_going (ecs);
+ /* We're doing a "next", set a breakpoint at callee's return
+	     address (the address at which the caller will
+	     resume).  */
+	  if (get_prev_frame ( get_current_frame ()))
+	    {
+	      insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ()));
+	    }
+	  else
+	    {
+	        struct symtab_and_line sr_sal;
+		init_sal (&sr_sal);		/* initialize to zeros */
+		sr_sal.pc = ADDR_BITS_REMOVE (gdbarch_unwind_pc (current_gdbarch,get_current_frame()));
+		sr_sal.section = find_pc_overlay (sr_sal.pc);
+		insert_step_resume_breakpoint_at_sal (sr_sal, null_frame_id);
+	    }   
+	      keep_going (ecs);
 	  return;
 	}
 

Re: Fix for PR 1971 .

[Jim Blandy]

On 1/3/06, Ramana Radhakrishnan <ramana.radhakrishnan@codito.com> wrote:
> Attached is a fix for PR 1971. This inserts a breakpoint at the return
> address for a function that does not have a previous frame which is what
> you have in the case of main. This would however not stop after the
> return from main because the semantics of the next command would not
> stop the execution in any place where there is no debug information.
>
> Tested on native x86 with today's head as well as 6.4 branch with no
> extra regressions .

Is this the behavior we actually want?  Where the user hasn't "set
backtrace past-main on", isn't it the correct behavior for GDB to
allow the program to exit when doing a 'next' out of main?  (I assume
that, if one does a 'set backtrace past-main on', then 'next' works as
you suggest it should.)

Re: Fix for PR 1971 .

[Jim Blandy]

On 1/3/06, Jim Blandy <jimb@red-bean.com> wrote:
> On 1/3/06, Ramana Radhakrishnan <ramana.radhakrishnan@codito.com> wrote:
> > Attached is a fix for PR 1971. This inserts a breakpoint at the return
> > address for a function that does not have a previous frame which is what
> > you have in the case of main. This would however not stop after the
> > return from main because the semantics of the next command would not
> > stop the execution in any place where there is no debug information.
> >
> > Tested on native x86 with today's head as well as 6.4 branch with no
> > extra regressions .
>
> Is this the behavior we actually want?  Where the user hasn't "set
> backtrace past-main on", isn't it the correct behavior for GDB to
> allow the program to exit when doing a 'next' out of main?  (I assume
> that, if one does a 'set backtrace past-main on', then 'next' works as
> you suggest it should.)

Oh, wait.  I might understand better now.  The current behavior is a
segfault, as get_prev_frame returns NULL.  The code you patched is
clearly wrong as it stands, since it doesn't account for that
possibility.  The behavior with your patch is that 'next' from main
causes the program to run to completion when past-main is off, as I
suggested it ought.  Right?

I see three uses of insert_step_resume_breakpoint_at_frame
(get_prev_frame (get_current_frame ())) in infrun.c; would it be
reasonable to add a new function in the
insert_step_resume_breakpoint_at_* family,
insert_step_resume_breakpoint_at_caller (say), that sets the
step-resume breakpoint at a sal built from the given frame's return
address?

I think you'd want to use frame_pc_unwind in that function, and not
gdbarch_unwind_pc.

Re: Fix for PR 1971 .

[Ramana Radhakrishnan]

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

Hi Jim, 


> > > Attached is a fix for PR 1971. This inserts a breakpoint at the return
> > > address for a function that does not have a previous frame which is what
> > > you have in the case of main. This would however not stop after the
> > > return from main because the semantics of the next command would not
> > > stop the execution in any place where there is no debug information.
> > >
> > > Tested on native x86 with today's head as well as 6.4 branch with no
> > > extra regressions .
> >
> > Is this the behavior we actually want?  Where the user hasn't "set
> > backtrace past-main on", isn't it the correct behavior for GDB to
> > allow the program to exit when doing a 'next' out of main?  (I assume
> > that, if one does a 'set backtrace past-main on', then 'next' works as
> > you suggest it should.)
> 
> Oh, wait.  I might understand better now.  The current behavior is a
> segfault, as get_prev_frame returns NULL.  The code you patched is
> clearly wrong as it stands, since it doesn't account for that
> possibility.  The behavior with your patch is that 'next' from main
> causes the program to run to completion when past-main is off, as I
> suggested it ought.  Right?

Right . Thats the intended behaviour. The debuggee would run to
completion depending  on whether there exists debug info beyond main or
not. 

> I see three uses of insert_step_resume_breakpoint_at_frame
> (get_prev_frame (get_current_frame ())) in infrun.c; would it be
> reasonable to add a new function in the
> insert_step_resume_breakpoint_at_* family,
> insert_step_resume_breakpoint_at_caller (say), that sets the
> step-resume breakpoint at a sal built from the given frame's return
> address?
> 
> I think you'd want to use frame_pc_unwind in that function, and not
> gdbarch_unwind_pc.
> 

 Here's an updated patch that uses a new function
insert_step_resume_breakpoint_at_caller and frame_pc_unwind as suggested
which has been tested on x86-linux with an extra pass in
watchthreads.exp, I am still not sure why this gets fixed by this.. 

 Ok to commit ? 

2006-01-04  Ramana Radhakrishnan  <ramana.radhakrishnan@codito.com>

       PR 1971
       * infrun.c (insert_step_resume_breakpoint_at_caller):New
function.
       (handle_inferior_event):Use above.


cheers
Ramana
-- 
Ramana Radhakrishnan
GNU Tools
codito ergo sum (www.codito.com)

[-- Attachment #2: pr1971-take2 --]
[-- Type: text/x-patch, Size: 2694 bytes --]

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.208
diff -u -a -u -r1.208 infrun.c
--- infrun.c	17 Dec 2005 22:34:01 -0000	1.208
+++ infrun.c	4 Jan 2006 04:40:11 -0000
@@ -943,6 +943,7 @@
 
 static void step_into_function (struct execution_control_state *ecs);
 static void insert_step_resume_breakpoint_at_frame (struct frame_info *step_frame);
+static void insert_step_resume_breakpoint_at_caller (struct frame_info *step_frame);
 static void insert_step_resume_breakpoint_at_sal (struct symtab_and_line sr_sal,
 						  struct frame_id sr_id);
 static void stop_stepping (struct execution_control_state *ecs);
@@ -2390,7 +2391,8 @@
 	  /* We're doing a "next", set a breakpoint at callee's return
 	     address (the address at which the caller will
 	     resume).  */
-	  insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ()));
+
+	  insert_step_resume_breakpoint_at_caller ( get_current_frame ());
 	  keep_going (ecs);
 	  return;
 	}
@@ -2453,7 +2455,7 @@
 
       /* Set a breakpoint at callee's return address (the address at
          which the caller will resume).  */
-      insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ()));
+      insert_step_resume_breakpoint_at_caller ( get_current_frame ());
       keep_going (ecs);
       return;
     }
@@ -2522,7 +2524,7 @@
 	{
 	  /* Set a breakpoint at callee's return address (the address
 	     at which the caller will resume).  */
-	  insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ()));
+	  insert_step_resume_breakpoint_at_caller ( get_current_frame ());
 	  keep_going (ecs);
 	  return;
 	}
@@ -2757,6 +2759,30 @@
   insert_step_resume_breakpoint_at_sal (sr_sal, get_frame_id (return_frame));
 }
 
+/* Insert a step resume breakpoint at the return address of the
+   caller. This is to ensure that on doing a next from before main completes
+   execution of the program without GDB dumping core. Look at PR 1971
+   for more details.  */
+
+static void 
+insert_step_resume_breakpoint_at_caller (struct frame_info *return_frame)
+{
+  if (get_prev_frame (return_frame))
+    {
+      insert_step_resume_breakpoint_at_frame (get_prev_frame (return_frame));
+    }
+  else
+    {
+      struct symtab_and_line sr_sal;
+      init_sal (&sr_sal);		/* initialize to zeros */
+      sr_sal.pc = ADDR_BITS_REMOVE (frame_pc_unwind (return_frame));
+      sr_sal.section = find_pc_overlay (sr_sal.pc);
+      insert_step_resume_breakpoint_at_sal (sr_sal, null_frame_id);
+    }   
+  
+}
+
+
 static void
 stop_stepping (struct execution_control_state *ecs)
 {

Re: Fix for PR 1971 .

[Jim Blandy]

Hi --- thanks for revising the patch.

+/* Insert a step resume breakpoint at the return address of the
+   caller. This is to ensure that on doing a next from before main completes
+   execution of the program without GDB dumping core. Look at PR 1971
+   for more details.  */

I don't think this is quite what you mean to say.  The comment for a
function should describe its behavior in terms of its arguments;
"caller" is vague.  Second, this isn't always used for "next"; it can
get used when you "step" into a function with no debug info (if I'm
reading right).  Third, a more self-contained explanation of why the
get_prev_frame-based approach isn't sufficient would be better.

How about:

/* Insert a step-resume breakpoint at the address to which
   RETURN_FRAME will return.

   Unlike insert_step_resume_breakpoint_at_frame (get_prev_frame (F)),
   this works even when F is the oldest frame.  (Consider next-ing out
   of main when 'show backtrace past-main' is off.)  */

Finally --- could insert_step_resume_breakpoint_at_caller simply
*always* use the frame_pc_unwind approach, and drop the check for
get_prev_frame altogether?  I understand that trying get_prev_frame
first is guaranteed to give the least disturbance to the existing
behavior, but I don't like leaving stuff like that in there unless we
can explain why it's actually needed.

Re: Fix for PR 1971 .

[Ramana Radhakrishnan]

Hi Jim 

I am not sure about removing the get_prev_frame. We need it for the
correct frame id .  In case you were stepping over a recursive call and
deep inside after main had executed  you would need the correct frame id
of the return frame and in the other case a null_frame_id.  

Changing over breaks the behaviour of the backtrace over a recursive
call. Gives me  extra failures inside break.exp . 

cheers
Ramana

On Tue, 2006-01-03 at 21:55 -0800, Jim Blandy wrote:
> Hi --- thanks for revising the patch.
> 
> +/* Insert a step resume breakpoint at the return address of the
> +   caller. This is to ensure that on doing a next from before main completes
> +   execution of the program without GDB dumping core. Look at PR 1971
> +   for more details.  */
> 
> I don't think this is quite what you mean to say.  The comment for a
> function should describe its behavior in terms of its arguments;
> "caller" is vague.  Second, this isn't always used for "next"; it can
> get used when you "step" into a function with no debug info (if I'm
> reading right).  Third, a more self-contained explanation of why the
> get_prev_frame-based approach isn't sufficient would be better.
> 
> How about:
> 
> /* Insert a step-resume breakpoint at the address to which
>    RETURN_FRAME will return.
> 
>    Unlike insert_step_resume_breakpoint_at_frame (get_prev_frame (F)),
>    this works even when F is the oldest frame.  (Consider next-ing out
>    of main when 'show backtrace past-main' is off.)  */
> 
> Finally --- could insert_step_resume_breakpoint_at_caller simply
> *always* use the frame_pc_unwind approach, and drop the check for
> get_prev_frame altogether?  I understand that trying get_prev_frame
> first is guaranteed to give the least disturbance to the existing
> behavior, but I don't like leaving stuff like that in there unless we
> can explain why it's actually needed.
-- 
Ramana Radhakrishnan
GNU Tools
codito ergo sum (www.codito.com)
Ph: +91-20-26051367 (office) +91-9890040009 (mobile)

Re: Fix for PR 1971 .

[Jim Blandy]

On 1/3/06, Ramana Radhakrishnan <ramana.radhakrishnan@codito.com> wrote:
> I am not sure about removing the get_prev_frame. We need it for the
> correct frame id .  In case you were stepping over a recursive call and
> deep inside after main had executed  you would need the correct frame id
> of the return frame and in the other case a null_frame_id.

Okay, I see.

Hmm.  If I'm reading breakpoint.c right, null_frame_id acts as a
wildcard, saying, treat the breakpoint as applying to any frame.  So
we're assuming that, whoever the caller of the "oldest" frame is, it's
not going to be called recursively (or else the step-resume breakpoint
would trigger in one of the inner frames).  I don't know if that case
is worth worrying about.

It looks okay to me, then.  Let's let it sit for a few days; if nobody
has futher comments, go ahead and commit.

Re: Fix for PR 1971 .

[Ramana Radhakrishnan]

On Tue, 2006-01-03 at 23:57 -0800, Jim Blandy wrote:
> On 1/3/06, Ramana Radhakrishnan <ramana.radhakrishnan@codito.com> wrote:
> > I am not sure about removing the get_prev_frame. We need it for the
> > correct frame id .  In case you were stepping over a recursive call and
> > deep inside after main had executed  you would need the correct frame id
> > of the return frame and in the other case a null_frame_id.
> 
> Okay, I see.
> 
> Hmm.  If I'm reading breakpoint.c right, null_frame_id acts as a
> wildcard, saying, treat the breakpoint as applying to any frame.  So
> we're assuming that, whoever the caller of the "oldest" frame is, it's
> not going to be called recursively (or else the step-resume breakpoint
> would trigger in one of the inner frames).  I don't know if that case
> is worth worrying about.


Thanks for all the review and the comments. Yes, that sounds right, I
can't think of a situation where we should worry about this.  Will wait
a couple of days and if there are no objections I will go ahead and
commit it. 

cheers
Ramana



> 
> It looks okay to me, then.  Let's let it sit for a few days; if nobody
> has futher comments, go ahead and commit.
> 

Re: Fix for PR 1971 .

[Daniel Jacobowitz]

On Wed, Jan 04, 2006 at 10:32:36AM +0530, Ramana Radhakrishnan wrote:
> 2006-01-04  Ramana Radhakrishnan  <ramana.radhakrishnan@codito.com>
> 
>        PR 1971
>        * infrun.c (insert_step_resume_breakpoint_at_caller):New
> function.
>        (handle_inferior_event):Use above.

Looks reasonable to me, but you need to correct some formatting
problems first.  Thanks for fixing this.

The ChangeLog entry should be:

2006-01-04  Ramana Radhakrishnan  <ramana.radhakrishnan@codito.com>

	PR 1971
	* infrun.c (insert_step_resume_breakpoint_at_caller): New function.
	(handle_inferior_event): Use above.

Leading tabs, and a space after the colon.

> @@ -2390,7 +2391,8 @@
>  	  /* We're doing a "next", set a breakpoint at callee's return
>  	     address (the address at which the caller will
>  	     resume).  */
> -	  insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ()));
> +
> +	  insert_step_resume_breakpoint_at_caller ( get_current_frame ());

No space after the open parenthesis please (here and several times
below).

> +  if (get_prev_frame (return_frame))
> +    {
> +      insert_step_resume_breakpoint_at_frame (get_prev_frame (return_frame));
> +    }

No need for the extra braces here.

> +  else
> +    {
> +      struct symtab_and_line sr_sal;
> +      init_sal (&sr_sal);		/* initialize to zeros */

/* Initialize to zeros.  */

-- 
Daniel Jacobowitz
CodeSourcery