[09/12] detach and breakpoint removal

Message ID 20210113011543.2047449-10-pedro@palves.net
State New
Headers show
Series
  • Fix detach + displaced-step regression + N bugs more
Related show

Commit Message

Pedro Alves Jan. 13, 2021, 1:15 a.m.
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.
---
 gdb/linux-nat.c |  5 +++++
 gdb/remote.c    | 10 ++++++++++
 gdb/target.c    |  9 ---------
 3 files changed, 15 insertions(+), 9 deletions(-)

-- 
2.26.2

Comments

Lancelot SIX via Gdb-patches Jan. 13, 2021, 6:32 a.m. | #1
On 2021-01-12 8:15 p.m., Pedro Alves wrote:
> 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.


I think that makes sense.

I initially thought that GDB could always stop all threads, remove
breakpoints, and then ask the target to detach.  But then that's
unfair for the targets that can remove the breakpoints while all
threads are running.  These targets would pay a price (a window of
time where threads are paused when detaching) they don't need to pay.
So leaving the task of removing breakpoints to the target makes sense.

I think it would be good to mention this in the doc of
target_ops::detach (which doesn't exist yet), that the target is
responsible for removing the breakpoints from the inferior.

Simon
Pedro Alves Feb. 3, 2021, 1:28 a.m. | #2
On 13/01/21 06:32, Simon Marchi wrote:
> 

> 

> On 2021-01-12 8:15 p.m., Pedro Alves wrote:

>> 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.

> 

> I think that makes sense.

> 

> I initially thought that GDB could always stop all threads, remove

> breakpoints, and then ask the target to detach.  But then that's

> unfair for the targets that can remove the breakpoints while all

> threads are running.  These targets would pay a price (a window of

> time where threads are paused when detaching) they don't need to pay.

> So leaving the task of removing breakpoints to the target makes sense.


Yeah, that was my thinking too.  Always stopping everything would be
simpler, but with a potential penalty.

> 

> I think it would be good to mention this in the doc of

> target_ops::detach (which doesn't exist yet), that the target is

> responsible for removing the breakpoints from the inferior.

> 


I've merged the patch with this added comment:

+
+    /* 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 ();
+

Patch

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index dc524cf10dc..e46fa1ad6ff 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 7199e0ac422..423cd4a84c9 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5735,6 +5735,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