[PATCH 09/12] detach and breakpoint removal

[Pedro Alves]

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