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

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

[Andrew Burgess]

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.
---
 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
 	  }
 
-- 
2.25.4

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

[John Baldwin]

On 12/29/21 6:40 AM, 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.

This looks fine to me.

-- 
-- 
John Baldwin

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

[Tom de Vries]

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"' ?

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
>  	  }
>  
> 

Re: [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
> >  	  }
> >  
> > 
> 

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

[Tom de Vries]

On 1/4/22 11:24, Andrew Burgess wrote:
> * 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.
> 

Ah, right.

> 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.
> 

Got it, thanks for the explanation.

Thanks,
- Tom