Re: [PATCH] gdb: Don't ignore all SIGSTOP when the signal handler is set to pass

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Pedro Alves <palves@redhat.com> [2019-10-03 13:59:14 +0100]:

> On 9/10/19 6:37 PM, Andrew Burgess wrote:
> > It was observed that in a multi-threaded application on GNU/Linux,
> > that if the user has set the SIGSTOP to be pass (using GDB's handle
> > command) then the inferior would hang upon hitting a breakpoint.
> > 
> > What happens is that when a thread hits the breakpoint GDB tries to
> > stop all of the other threads by sending them a SIGSTOP and setting
> > the stop_requested flag in the target_ops structure - this can be seen
> > in infrun.c:stop_all_threads.
> > 
> > GDB then waits for all of the other threads to stop.
> > 
> > When the SIGSTOP event arrives we eventually end up in
> > linux-nat.c:linux_nat_filter_event, which has the job of deciding if
> > the event we're looking at (the SIGSTOP arriving in this case) is
> > something that should be reported back to the core of GDB.
> > 
> > One of the final actions of this function is to check if we stopped
> > due to a signal, and if we did, and the signal has been set to 'pass'
> > by the user then we ignore the event and resume the thread.
> > 
> > This code already has some conditions in place that mean the event is
> > reported to GDB even if the signal is in the set of signals to be
> > passed to the inferior.
> > 
> > In this commit I extend this condition to such that:
> > 
> >   If the signal is a SIGSTOP, and the thread's stop_requested flag is
> >   set (indicating we're waiting for the thread to stop with a SIGSTOP)
> >   then we should report this SIGSTOP to GDB and not pass it to the
> >   inferior.
> > 
> > With this change in place the test now passes.  Regression tested on
> > x86-64 GNU/Linux with no regressions.
> 
> Indeed, makes sense.  
> 
> I checked that gdbserver passes the new test too.  (It knows to always
> report an event when gdb tries to stop a thread with 'vCont;t'.)
> 
> > +/* The worker thread just spins forever.  */
> > +void*
> > +thread_worker (void *payload)
> > +{
> > +  while (1)
> > +    {
> > +      sleep (1);
> > +    }
> > +
> > +  return NULL;
> > +}
> > +
> 
> > +/* A place for GDB to place a breakpoint.   */
> > +void
> > +breakpt ()
> > +{
> > +  asm ("" ::: "memory");
> 
> I wonder whether this barrier is a good idea, given
> that it limits portability to gcc and clang.
> 
> I checked that both clang and gcc still manage to
> eliminate the function with "-flto -O2", the barrier
> didn't help then.  '__attribute__((used))' instead
> of the barrier works though.
> 
> > +}
> 
> > +
> > +/* Create a worker thread that just spins forever, then enter a loop
> > +   periodically calling the BREAKPT function.  */
> > +int main() {
> 
> GNU formatting.
> 
> > +  spawn_thread ();
> > +
> > +  while (1)
> > +    {
> > +      sleep (1);
> > +
> > +      breakpt ();
> > +    }
> > +
> > +  return 0;
> > +}
> 
> It's better to avoid that the testcase really runs forever, in case GDB
> crashes and the system doesn't kill the process.  Easily fixed with
> an "alarm" call at the top of main.
> 
> LGTM otherwise.
> 
> BTW, FYI, I saw this:
> 
> $ git am /tmp/sigstop.mbox
> Applying: gdb: Don't ignore all SIGSTOP when the signal handler is set to pass
> .git/rebase-apply/patch:104: new blank line at EOF.
> +
> .git/rebase-apply/patch:162: new blank line at EOF.
> +
> warning: 2 lines add whitespace errors.
> 

I pushed this patch with the fixes you identified above.

Thanks,
Andrew