From e87f0fe82375ffe508da44172fcfe04e258ddc9c Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Sun, 13 Dec 2020 01:35:05 +0000 Subject: [PATCH] detach and breakpoint removal A following patch will add a testcase that has a number of threads constantly stepping over a breakpoint, and then has GDB detach the process. That testcase sometimes fails with the inferior crashing with SIGTRAP after the detach because of the bug fixed by this patch, when tested with the native target. The problem is that target_detach removes breakpoints from the target immediately, and that does not work with the native GNU/Linux target (and probably no other native target) currently. The test wouldn't fail with this issue when testing against gdbserver, because gdbserver does allow accessing memory while the current thread is running, by transparently pausing all threads temporarily, without GDB noticing. Implementing that in gdbserver was a lot of work, so I'm not looking forward right now to do the same in the native target. Instead, I came up with a simpler solution -- push the breakpoints removal down to the targets. The Linux target conveniently already pauses all threads before detaching them, since PTRACE_DETACH only works with stopped threads, so we move removing breakpoints to after that. Only the remote and GNU/Linux targets support support async execution, so no other target should really need this. gdb/ChangeLog: * linux-nat.c (linux_nat_target::detach): Remove breakpoints here... * remote.c (remote_target::remote_detach_1): ... and here ... * target.c (target_detach): ... instead of here. * target.h (target_ops::detach): Add comment. --- gdb/ChangeLog | 8 ++++++++ gdb/linux-nat.c | 5 +++++ gdb/remote.c | 10 ++++++++++ gdb/target.c | 9 --------- gdb/target.h | 7 +++++++ 5 files changed, 30 insertions(+), 9 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 62aab57972f..4113e3a6284 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,11 @@ +2021-02-03 Pedro Alves + + * linux-nat.c (linux_nat_target::detach): Remove breakpoints + here... + * remote.c (remote_target::remote_detach_1): ... and here ... + * target.c (target_detach): ... instead of here. + * target.h (target_ops::detach): Add comment. + 2021-02-03 Pedro Alves * infrun.c (struct wait_one_event): Move higher up. diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c index 3c7117bff6e..10419dc7bb5 100644 --- a/gdb/linux-nat.c +++ b/gdb/linux-nat.c @@ -1456,6 +1456,11 @@ linux_nat_target::detach (inferior *inf, int from_tty) they're no longer running. */ iterate_over_lwps (ptid_t (pid), stop_wait_callback); + /* We can now safely remove breakpoints. We don't this in earlier + in common code because this target doesn't currently support + writing memory while the inferior is running. */ + remove_breakpoints_inf (current_inferior ()); + iterate_over_lwps (ptid_t (pid), detach_callback); /* Only the initial process should be left right now. */ diff --git a/gdb/remote.c b/gdb/remote.c index 512bd9467f6..c544fe7fcec 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -5800,6 +5800,16 @@ remote_target::remote_detach_1 (inferior *inf, int from_tty) target_announce_detach (from_tty); + if (!gdbarch_has_global_breakpoints (target_gdbarch ())) + { + /* If we're in breakpoints-always-inserted mode, or the inferior + is running, we have to remove breakpoints before detaching. + We don't do this in common code instead because not all + targets support removing breakpoints while the target is + running. The remote target / gdbserver does, though. */ + remove_breakpoints_inf (current_inferior ()); + } + /* Tell the remote target to detach. */ remote_detach_pid (pid); diff --git a/gdb/target.c b/gdb/target.c index 3a03a0ad530..9a8473d40e1 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -1949,15 +1949,6 @@ target_detach (inferior *inf, int from_tty) assertion. */ gdb_assert (inf == current_inferior ()); - if (gdbarch_has_global_breakpoints (target_gdbarch ())) - /* Don't remove global breakpoints here. They're removed on - disconnection from the target. */ - ; - else - /* If we're in breakpoints-always-inserted mode, have to remove - breakpoints before detaching. */ - remove_breakpoints_inf (current_inferior ()); - prepare_for_detach (); /* Hold a strong reference because detaching may unpush the diff --git a/gdb/target.h b/gdb/target.h index 917476d16ab..c97ef690e01 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -470,8 +470,15 @@ struct target_ops virtual void attach (const char *, int); virtual void post_attach (int) TARGET_DEFAULT_IGNORE (); + + /* Detaches from the inferior. Note that on targets that support + async execution (i.e., targets where it is possible to detach + from programs with threads running), the target is responsible + for removing breakpoints from the program before the actual + detach, otherwise the program dies when it hits one. */ virtual void detach (inferior *, int) TARGET_DEFAULT_IGNORE (); + virtual void disconnect (const char *, int) TARGET_DEFAULT_NORETURN (tcomplain ()); virtual void resume (ptid_t, -- 2.30.2