Re: RFA: fix PR breakpoints/13776

Pedro Alves
2012-02-29T19:58:39Z

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