[review v4] Add RAII class for blocking gdb signals

[Pedro Alves (Code Review)]

Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/168
......................................................................


Patch Set 4:

(2 comments)

| --- /dev/null
| +++ gdb/gdbsupport/block-signals.h
| @@ -1,0 +34,19 @@ public:
| +  block_signals ()
| +  {
| +#ifdef HAVE_PTHREAD_SIGMASK
| +    sigset_t mask;
| +    sigemptyset (&mask);
| +    sigaddset (&mask, SIGINT);
| +    sigaddset (&mask, SIGCHLD);
| +    sigaddset (&mask, SIGALRM);
| +    sigaddset (&mask, SIGWINCH);
| +    pthread_sigmask (SIG_BLOCK, &mask, &m_old_mask);

PS3, Line 43:

> Well, I thought perhaps not, since this class should only really do anything
> when using threads.  However, it's also harmless to change this, and
> I suppose clearer -- so I have made the change.

Yeah, both the file and class name give no hint that this is threads
related.  Also, the intro comment says that it can be used when
starting threads, but it doesn't say that it can't be used in whatever
other cases we might want to block signals.

I actually wondered whether this should block all signals instead of
just blocking a subset -- I was pondering whether the set of signals
might be target backend specific, for example.  But we can leave that
for if/when we need it.

| +#endif
| +  }
| +
| +  ~block_signals ()
| +  {
| +#ifdef HAVE_PTHREAD_SIGMASK
| +    pthread_sigmask (SIG_SETMASK, &m_old_mask, nullptr);
| +#endif
| +  }
| --- gdb/gdbsupport/signals-state-save-restore.c
| +++ gdb/gdbsupport/signals-state-save-restore.c
| @@ -20,21 +20,18 @@ #include "signals-state-save-restore.h"
| +#include "gdbsupport/gdb-sigmask.h"
|  
|  #include <signal.h>
|  
|  /* The original signal actions and mask.  */
|  
|  #ifdef HAVE_SIGACTION
|  static struct sigaction original_signal_actions[NSIG];
|  
| -/* Note that we use sigprocmask without worrying about threads because
| -   the save/restore functions are called either from main, or after a
| -   fork.  In both cases, we know the calling process is single
| -   threaded.  */

PS3, Line 31:

> I rewrote the comment.

I'm afraid I don't understand the new comment.  :-(

Maybe we should remove it afterall.

|  static sigset_t original_signal_mask;
|  #endif
|  
|  /* See signals-state-save-restore.h.   */
|  
|  void
|  save_original_signals_state (bool quiet)
|  {
|  #ifdef HAVE_SIGACTION

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: If3f37dc57dd859c226e9e4d79458a0514746e8c6
Gerrit-Change-Number: 168
Gerrit-PatchSet: 4
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 26 Nov 2019 15:41:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Comment-In-Reply-To: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: comment