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

Andrew Burgess
2019-10-03T15:14:40Z

* 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