Re: RFA: fix PR breakpoints/13776

[Pedro Alves <palves at redhat dot com>]

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