[PATCH] gdb: don't pass nullptr to sigwait

[Andrew Burgess]

* Tom de Vries <tdevries@suse.de> [2022-01-02 11:55:49 +0100]:

> On 12/29/21 3:40 PM, Andrew Burgess via Gdb-patches wrote:
> > I tried building GDB on GNU/Hurd, and ran into this warning:
> > 
> >   gdbsupport/scoped_ignore_signal.h:78:16: error: null argument where non-null required (argument 2) [-Werror=nonnull]
> > 
> > This is because in this commit:
> > 
> >   commit 99624310dd82542c389c89c2e55d8cae36bb74e1
> >   Date:   Sun Jun 27 15:13:14 2021 -0400
> > 
> >       gdb: fall back on sigpending + sigwait if sigtimedwait is not available
> > 
> > A call to sigwait was introduced that passes nullptr as the second
> > argument, this call is only reached if sigtimedwait is not supported.
> > 
> > The original patch was written for macOS, I assume on that target
> > passing nullptr as the second argument is fine.
> > 
> > On my GNU/Linux box, the man-page for sigwait doesn't mention that
> > nullptr is allowed for the second argument, so my assumption would be
> > that nullptr is not OK, and, if I change the '#ifdef
> > HAVE_SIGTIMEDWAIT' introduced by the above patch to '#if 0', and
> > rebuild on GNU/Linux, I see the same warning that I see on GNU/Hurd.
> > 
> > I propose that we stop passing nullptr as the second argument to
> > sigwait, and instead pass a valid int pointer.  The value returned in
> > the int can then be used in an assert.
> > 
> > For testing, I (locally) made the change to the #ifdef I mentioned
> > above, compiled GDB, and ran the usual tests, this meant I was using
> > sigwait instead on sigtimedwait on GNU/Linux, I saw no regressions.
> > ---
> 
> Hi,
> 
> the patch LGTM.
> 
> I do wonder though: I disabled the nonnull attribute in commit
> fb6262e8534 ("[gdb/build] Disable attribute nonnull"), so does the fact
> that this warning triggers mean that some file is missing an '#include
> "gdbsupport/common-defs.h"' ?

No, common-defs.h is being included correctly.  Commit fb6262e8534
only disables the #define ATTRIBUTE_NONNULL.  This makes sense, as the
problem you were addressing was GDB functions being marked as nonnull,
but then also including a nullptr assertion check (which was optimised
out).

In this case, it is sigwait itself that is marked nonull, and this
declaration is in a standard header file, and so doesn't use
ATTRIBUTE_NONNULL, but uses the underlying attribute directly.

This is, I think, what we want here.  At the very least, if the
underlying c-library does want to include a nullptr assert type check
then they will hit the same problems we hit in GDB, but that's not
something we (as GDB devs) needs to worry about.

I'm going to go ahead and push this patch.

Thanks,
Andrew



> 
> Thanks,
> - Tom
> 
> >  gdbsupport/scoped_ignore_signal.h | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/gdbsupport/scoped_ignore_signal.h b/gdbsupport/scoped_ignore_signal.h
> > index 57dd4b6d402..6e69044128c 100644
> > --- a/gdbsupport/scoped_ignore_signal.h
> > +++ b/gdbsupport/scoped_ignore_signal.h
> > @@ -75,7 +75,12 @@ class scoped_ignore_signal
> >  
> >  	    sigpending (&pending);
> >  	    if (sigismember (&pending, Sig))
> > -	      sigwait (&set, nullptr);
> > +	      {
> > +		int sig_found;
> > +
> > +		sigwait (&set, &sig_found);
> > +		gdb_assert (sig_found == Sig);
> > +	      }
> >  #endif
> >  	  }
> >  
> > 
>