RFA: fix PR breakpoints/13776

RFA: fix PR breakpoints/13776

[Tom Tromey]

I'd appreciate comments on this patch.
I have no idea whether it is the best way to fix the problem.

Bug 13776 concerns 'next'ing over an exit.  For the trivial:

    #include <stdlib.h> 
    int
    main (void)
    {
      exit (0);
    }

We get this behavior:

    (gdb) start
    Temporary breakpoint 1, main () at exit0.c:5
    5      exit (0);
    (gdb) next
    [Inferior 1 (process 2428) exited normally]
    warning: Error removing breakpoint 0
    warning: Error removing breakpoint 0
    warning: Error removing breakpoint 0

The bug is that exit_inferior ends up calling delete_longjmp_breakpoint,
which tries to delete the longjmp breakpoints -- but as the inferior is
dead, this fails.

This patch fixes this problem by moving the breakpoint_init_inferior
call earlier in generic_mourn_inferior.  This causes the breakpoints to
be marked as uninserted before they are deleted.

While doing this I noticed that after the inferior exits, we are left
with a step-resume breakpoint:

(gdb) maint info b
Num     Type           Disp Enb Address            What
[...]
0       step resume    dstp y   0x00000000004004d2  inf 1 thread 1
	stop only in thread 1

The breakpoint.c patch causes this to be removed as well.

Built and regtested on x86-64 Fedora 16.

Tom

2012-02-28  Tom Tromey  <tromey@redhat.com>

	PR breakpoints/13776:
	* target.c (generic_mourn_inferior): Call breakpoint_init_inferior
	earlier.
	* breakpoint.c (breakpoint_init_inferior): Delete step-resume
	breakpoints.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index db05b97..048cc63 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3341,6 +3341,10 @@ breakpoint_init_inferior (enum inf_context context)
 	   (gdb) tar rem :9999     # remote Windows gdbserver.
 	*/
 
+      case bp_step_resume:
+
+	/* Also remove step-resume breakpoints.  */
+
 	delete_breakpoint (b);
 	break;
 
diff --git a/gdb/target.c b/gdb/target.c
index 1f408f6..65a6c23 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3583,13 +3583,14 @@ generic_mourn_inferior (void)
   ptid = inferior_ptid;
   inferior_ptid = null_ptid;
 
+  breakpoint_init_inferior (inf_exited);
+
   if (!ptid_equal (ptid, null_ptid))
     {
       int pid = ptid_get_pid (ptid);
       exit_inferior (pid);
     }
 
-  breakpoint_init_inferior (inf_exited);
   registers_changed ();
 
   reopen_exec_file ();

Re: RFA: fix PR breakpoints/13776

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> I'd appreciate comments on this patch.
Tom> I have no idea whether it is the best way to fix the problem.

I somehow forgot to turn the test case into a patch against the test
suite.

So, please look at the patch; but know that I will not check anything in
until the test case is written up.

thanks,
Tom

Re: RFA: fix PR breakpoints/13776

[Jan Kratochvil]

On Tue, 28 Feb 2012 18:26:48 +0100, Tom Tromey wrote:
> I'd appreciate comments on this patch.
> I have no idea whether it is the best way to fix the problem.

I find it OK although I already made some such mistakes in the past.


Thanks,
Jan

Re: RFA: fix PR breakpoints/13776

[Doug Evans]

On Tue, Feb 28, 2012 at 9:26 AM, Tom Tromey <tromey@redhat.com> wrote:
> I'd appreciate comments on this patch.
> [...]
> This patch fixes this problem by moving the breakpoint_init_inferior
> call earlier in generic_mourn_inferior.  This causes the breakpoints to
> be marked as uninserted before they are deleted.
> [...]
> diff --git a/gdb/target.c b/gdb/target.c
> index 1f408f6..65a6c23 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -3583,13 +3583,14 @@ generic_mourn_inferior (void)
>   ptid = inferior_ptid;
>   inferior_ptid = null_ptid;
>
> +  breakpoint_init_inferior (inf_exited);
> +
>   if (!ptid_equal (ptid, null_ptid))
>     {
>       int pid = ptid_get_pid (ptid);
>       exit_inferior (pid);
>     }
>
> -  breakpoint_init_inferior (inf_exited);
>   registers_changed ();
>
>   reopen_exec_file ();

A comment right before the new breakpoint_init_inferior call to
explain why it lives there would be useful to future readers of the
code.

Re: RFA: fix PR breakpoints/13776

[Pedro Alves]

On 02/28/2012 05:26 PM, Tom Tromey wrote:
> I'd appreciate comments on this patch.
> I have no idea whether it is the best way to fix the problem.
> 
> Bug 13776 concerns 'next'ing over an exit.  For the trivial:
> 
>     #include <stdlib.h> 
>     int
>     main (void)
>     {
>       exit (0);
>     }
> 
> We get this behavior:
> 
>     (gdb) start
>     Temporary breakpoint 1, main () at exit0.c:5
>     5      exit (0);
>     (gdb) next
>     [Inferior 1 (process 2428) exited normally]
>     warning: Error removing breakpoint 0
>     warning: Error removing breakpoint 0
>     warning: Error removing breakpoint 0
> 
> The bug is that exit_inferior ends up calling delete_longjmp_breakpoint,
> which tries to delete the longjmp breakpoints -- but as the inferior is
> dead, this fails.
> 
> This patch fixes this problem by moving the breakpoint_init_inferior
> call earlier in generic_mourn_inferior.  This causes the breakpoints to
> be marked as uninserted before they are deleted.
> 
> While doing this I noticed that after the inferior exits, we are left
> with a step-resume breakpoint:
> 
> (gdb) maint info b
> Num     Type           Disp Enb Address            What
> [...]
> 0       step resume    dstp y   0x00000000004004d2  inf 1 thread 1
> 	stop only in thread 1
> 
> The breakpoint.c patch causes this to be removed as well.
> 
> Built and regtested on x86-64 Fedora 16.
> 
> Tom
> 
> 2012-02-28  Tom Tromey  <tromey@redhat.com>
> 
> 	PR breakpoints/13776:
> 	* target.c (generic_mourn_inferior): Call breakpoint_init_inferior
> 	earlier.
> 	* breakpoint.c (breakpoint_init_inferior): Delete step-resume
> 	breakpoints.
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index db05b97..048cc63 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -3341,6 +3341,10 @@ breakpoint_init_inferior (enum inf_context context)
>  	   (gdb) tar rem :9999     # remote Windows gdbserver.
>  	*/
>  
> +      case bp_step_resume:
> +
> +	/* Also remove step-resume breakpoints.  */
> +
>  	delete_breakpoint (b);
>  	break;
>  
> diff --git a/gdb/target.c b/gdb/target.c
> index 1f408f6..65a6c23 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -3583,13 +3583,14 @@ generic_mourn_inferior (void)
>    ptid = inferior_ptid;
>    inferior_ptid = null_ptid;
>  
> +  breakpoint_init_inferior (inf_exited);
> +

If you do this before exit_inferior, then the
breakpoint_init_inferior change is wrong, because the
clear_thread_inferior_resources will touch already released memory.

Note with valgrind:

main () at stepexit.c:6
6         exit (0);
(gdb) n
[Inferior 1 (process 11456) exited normally]
==11452== Invalid write of size 4
==11452==    at 0x5B38C2: clear_thread_inferior_resources (thread.c:105)
==11452==    by 0x5B3E62: delete_thread_1 (thread.c:294)
==11452==    by 0x5B3EC7: delete_thread (thread.c:311)
==11452==    by 0x6CDFC0: delete_thread_of_inferior (inferior.c:175)
==11452==    by 0x5B3FE8: iterate_over_threads (thread.c:368)
==11452==    by 0x6CE1C4: exit_inferior_1 (inferior.c:272)
==11452==    by 0x6CE25B: exit_inferior (inferior.c:295)
==11452==    by 0x5E542E: generic_mourn_inferior (target.c:3591)
==11452==    by 0x484C1B: inf_ptrace_mourn_inferior (inf-ptrace.c:181)
==11452==    by 0x491F19: linux_nat_mourn_inferior (linux-nat.c:4227)
==11452==    by 0x5E3F89: target_mourn_inferior (target.c:2755)
==11452==    by 0x5A3353: handle_inferior_event (infrun.c:3405)
==11452==  Address 0x4cba398 is 24 bytes inside a block of size 184 free'd
==11452==    at 0x4A0662E: free (vg_replace_malloc.c:366)
==11452==    by 0x6DB4F0: xfree (common-utils.c:107)
==11452==    by 0x5407EB: delete_breakpoint (breakpoint.c:12666)
==11452==    by 0x52FE08: breakpoint_init_inferior (breakpoint.c:3348)
==11452==    by 0x5E53BB: generic_mourn_inferior (target.c:3586)
==11452==    by 0x484C1B: inf_ptrace_mourn_inferior (inf-ptrace.c:181)
==11452==    by 0x491F19: linux_nat_mourn_inferior (linux-nat.c:4227)
==11452==    by 0x5E3F89: target_mourn_inferior (target.c:2755)
==11452==    by 0x5A3353: handle_inferior_event (infrun.c:3405)
==11452==    by 0x5A1CB5: wait_for_inferior (infrun.c:2709)
==11452==    by 0x5A0E9B: proceed (infrun.c:2278)
==11452==    by 0x599EFB: step_once (infcmd.c:1067)
==11452==
(gdb)


step-resume breakpoint lifetime isn't the prettiest thing...

>    if (!ptid_equal (ptid, null_ptid))
>      {
>        int pid = ptid_get_pid (ptid);
>        exit_inferior (pid);
>      }
>  
> -  breakpoint_init_inferior (inf_exited);
>    registers_changed ();
>  
>    reopen_exec_file ();

We can call mark_breakpoints_out before that exit_inferior to only
mark the breakpoints uninserted, while leaving the rest of the
breakpoint_init_inferior work do be done where it is.

Also note how we're careful to not delete step-resume and
exception resume breakpoints directly in clear_thread_inferior_resources.

static void
clear_thread_inferior_resources (struct thread_info *tp)
{
  /* NOTE: this will take care of any left-over step_resume breakpoints,
     but not any user-specified thread-specific breakpoints.  We can not
     delete the breakpoint straight-off, because the inferior might not
     be stopped at the moment.  */
  if (tp->control.step_resume_breakpoint)
    {
      tp->control.step_resume_breakpoint->disposition = disp_del_at_next_stop;
      tp->control.step_resume_breakpoint = NULL;
    }

  if (tp->control.exception_resume_breakpoint)
    {
      tp->control.exception_resume_breakpoint->disposition
	= disp_del_at_next_stop;
      tp->control.exception_resume_breakpoint = NULL;
    }

  bpstat_clear (&tp->control.stop_bpstat);

  do_all_intermediate_continuations_thread (tp, 1);
  do_all_continuations_thread (tp, 1);

  delete_longjmp_breakpoint (tp->num);
}

I think longjmp breakpoints should get the same treatment, though this
isn't strictly necessary for the exit case (though it is for
pthread_exit on targets without thread exit notifications, like remote).

With this, the problem is gone, in both sync and async modes, and we
have no step-resume breakpoint left behind.  Maybe we should also
delete longjmp and exception-resume breakpoints similarly in
breakpoint_init_inferior, but since they're always deleted either through
a cleanup or a continuation I couldn't find a way to leave them stale.
There's probably a way with some path that throws an error before the
continuation that deletes the breakpoint is reached.  I'm leaving that
out for now.

WDTY?

2012-02-29  Tom Tromey  <tromey@redhat.com>
	    Pedro Alves  <palves@redhat.com>

	PR breakpoints/13776:
	* breakpoint.c (breakpoint_init_inferior): Delete step-resume
	breakpoints.
	(delete_longjmp_breakpoint_at_next_stop): New.
	* breakpoint.h (delete_longjmp_breakpoint_at_next_stop): Declare.
	* target.c (generic_mourn_inferior): Call mark_breakpoints_out
	before deleting the inferior.  Add comments.
	* thread.c (clear_thread_inferior_resources): Don't delete lonjmp
	breakpoints immediately, but only on next stop.  Move that code
	next to where we mark other breakpoints for deletion.
---

 gdb/breakpoint.c |   17 +++++++++++++++++
 gdb/breakpoint.h |    3 +++
 gdb/target.c     |    9 +++++++++
 gdb/thread.c     |    4 ++--
 4 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index db05b97..f634d97 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3341,6 +3341,10 @@ breakpoint_init_inferior (enum inf_context context)
 	   (gdb) tar rem :9999     # remote Windows gdbserver.
 	*/

+      case bp_step_resume:
+
+	/* Also remove step-resume breakpoints.  */
+
 	delete_breakpoint (b);
 	break;

@@ -6625,6 +6629,19 @@ delete_longjmp_breakpoint (int thread)
 }

 void
+delete_longjmp_breakpoint_at_next_stop (int thread)
+{
+  struct breakpoint *b, *b_tmp;
+
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
+    if (b->type == bp_longjmp || b->type == bp_exception)
+      {
+	if (b->thread == thread)
+	  b->disposition = disp_del_at_next_stop;
+      }
+}
+
+void
 enable_overlay_breakpoints (void)
 {
   struct breakpoint *b;
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 7e8c597..5b5172e 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1254,6 +1254,9 @@ extern void set_longjmp_breakpoint (struct thread_info *tp,
 				    struct frame_id frame);
 extern void delete_longjmp_breakpoint (int thread);

+/* Mark all longjmp breakpoints from THREAD for later deletion.  */
+extern void delete_longjmp_breakpoint_at_next_stop (int thread);
+
 extern void enable_overlay_breakpoints (void);
 extern void disable_overlay_breakpoints (void);

diff --git a/gdb/target.c b/gdb/target.c
index 1f408f6..d29d37a 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3583,13 +3583,22 @@ generic_mourn_inferior (void)
   ptid = inferior_ptid;
   inferior_ptid = null_ptid;

+  /* Mark breakpoints uninserted in case something tries to delete a
+     breakpoint while we delete the inferior's threads (which would
+     fail, since the inferior is long gone).  */
+  mark_breakpoints_out ();
+
   if (!ptid_equal (ptid, null_ptid))
     {
       int pid = ptid_get_pid (ptid);
       exit_inferior (pid);
     }

+  /* Note this wipes step-resume breakpoints, so needs to be done
+     after exit_inferior, which ends up referencing the step-resume
+     breakpoints through clear_thread_inferior_resources.  */
   breakpoint_init_inferior (inf_exited);
+
   registers_changed ();

   reopen_exec_file ();
diff --git a/gdb/thread.c b/gdb/thread.c
index 22d8b23..97f283c 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -113,12 +113,12 @@ clear_thread_inferior_resources (struct thread_info *tp)
       tp->control.exception_resume_breakpoint = NULL;
     }

+  delete_longjmp_breakpoint_at_next_stop (tp->num);
+
   bpstat_clear (&tp->control.stop_bpstat);

   do_all_intermediate_continuations_thread (tp, 1);
   do_all_continuations_thread (tp, 1);
-
-  delete_longjmp_breakpoint (tp->num);
 }

 static void

-- 
Pedro Alves

Re: RFA: fix PR breakpoints/13776

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> If you do this before exit_inferior, then the
Pedro> breakpoint_init_inferior change is wrong, because the
Pedro> clear_thread_inferior_resources will touch already released memory.

Ugh, thanks for noticing.
I should have tried this.

Pedro> WDTY?

I think the patch is fine.

Pedro> 2012-02-29  Tom Tromey  <tromey@redhat.com>
Pedro> 	    Pedro Alves  <palves@redhat.com>

... but it is silly to put my name on it since I think I wrote one line
in the result :)

Here is a test case for the bug.
I can commit it first if you'd prefer.

Tom

2012-03-02  Tom Tromey  <tromey@redhat.com>

	* gdb.base/nextoverexit.c: New file.
	* gdb.base/nextoverexit.exp: New file.

diff --git a/gdb/testsuite/gdb.base/nextoverexit.c b/gdb/testsuite/gdb.base/nextoverexit.c
new file mode 100644
index 0000000..125d68f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/nextoverexit.c
@@ -0,0 +1,23 @@
+/* Copyright 2012 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include <stdlib.h> 
+
+int
+main (void)
+{
+  exit (0);
+}
diff --git a/gdb/testsuite/gdb.base/nextoverexit.exp b/gdb/testsuite/gdb.base/nextoverexit.exp
new file mode 100644
index 0000000..fecbd58
--- /dev/null
+++ b/gdb/testsuite/gdb.base/nextoverexit.exp
@@ -0,0 +1,33 @@
+# Copyright 2012 Free Software Foundation, Inc.
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set testfile "nextoverexit"
+set executable $testfile
+set srcfile $testfile.c
+set binfile $objdir/$subdir/$testfile
+
+if {[prepare_for_testing $testfile.exp $testfile $testfile.c]} {
+    return -1
+}
+
+if {![runto_main]} {
+    return -1
+}
+
+# Make sure we do not see any warnings.
+gdb_test_multiple "next" "next over exit" {
+    -re "$inferior_exited_re normally.\[\r\n\]+$gdb_prompt $" {
+	pass "next over exit"
+    }
+}

Re: RFA: fix PR breakpoints/13776

[Doug Evans]

On Fri, Mar 2, 2012 at 9:39 AM, Tom Tromey <tromey@redhat.com> wrote:
> ... but it is silly to put my name on it since I think I wrote one line
> in the result :)

[fwiw]
Au contraire.  There's more to it than just writing the final version
of the patch, and I would be uncomfortable with the potential
precedent.

Re: RFA: fix PR breakpoints/13776

[Pedro Alves]

On 03/02/2012 05:39 PM, Tom Tromey wrote:
> I think the patch is fine.
> 
> Pedro> 2012-02-29  Tom Tromey  <tromey@redhat.com>
> Pedro> 	    Pedro Alves  <palves@redhat.com>
> 
> ... but it is silly to put my name on it since I think I wrote one line
> in the result :)

Nah, my version is clearly derived from yours.  I've checked it in now.

> 
> Here is a test case for the bug.
> I can commit it first if you'd prefer.
> 
> Tom
> 
> 2012-03-02  Tom Tromey  <tromey@redhat.com>
> 
> 	* gdb.base/nextoverexit.c: New file.
> 	* gdb.base/nextoverexit.exp: New file.

Looks good to me.

-- 
Pedro Alves