Use an alias to `ptsname_r' in login/tst-ptsname.c

  When compiled with `-O3', inlining causes GCC to realize that one of the
tests calls `ptsname_r' with a NULL buffer and error out when compiling
with -Wnonnull.

Declare `ptsname_r_alias' and alias it to `ptsname_r' to avoid this.

ChangeLog:

2017-05-24  Arjun Shankar  <arjun.is@lostca.se>

	* login/tst-ptsname.c (ptsname_r_alias): New declaration.
	(do_single_test): Use ptsname_r_alias instead of ptsname_r.
---
 login/tst-ptsname.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
  

On Wed, 24 May 2017, Arjun Shankar wrote:

> When compiled with `-O3', inlining causes GCC to realize that one of the
> tests calls `ptsname_r' with a NULL buffer and error out when compiling
> with -Wnonnull.
> 
> Declare `ptsname_r_alias' and alias it to `ptsname_r' to avoid this.

Normally in such cases we'd use the DIAG_* macros from libc-diag.h, with 
appropriate comments, to disable the specific warning for the code in 
question.
  

On 05/24/2017 06:18 PM, Joseph Myers wrote:
> On Wed, 24 May 2017, Arjun Shankar wrote:
> 
>> When compiled with `-O3', inlining causes GCC to realize that one of the
>> tests calls `ptsname_r' with a NULL buffer and error out when compiling
>> with -Wnonnull.
>>
>> Declare `ptsname_r_alias' and alias it to `ptsname_r' to avoid this.
> 
> Normally in such cases we'd use the DIAG_* macros from libc-diag.h, with 
> appropriate comments, to disable the specific warning for the code in 
> question.

I think passing NULL to a nonnull parameter is undefined, warning or
not.  The GCC documentation is pretty clear about that.  (I have some
doubts about the validity of the NULL comparison inside the
implementation of ptsname_r, too.)

Alternatively, we could withdraw the test as invalid, and update the
manual page.

Thanks,
Florian
  

On Wed, May 24, 2017 at 07:42:37PM +0200, Florian Weimer wrote:
> I think passing NULL to a nonnull parameter is undefined, warning or
> not.  The GCC documentation is pretty clear about that.  (I have some
> doubts about the validity of the NULL comparison inside the
> implementation of ptsname_r, too.)
> 
> Alternatively, we could withdraw the test as invalid, and update the
> manual page.

Removing the sub-test that tests for EINVAL on NULL sounds like a good
idea to me as well. About the man-page I just looked at
`pthread_create' for comparison:

* the `pthread_t *' argument is declared `nonnull' in pthread.h
* there is no code in `pthread_create' to check if the argument is NULL
* nothing in the man page to indicate what happens when NULL is passed
* compiling `pthread_create (NULL, NULL, NULL, NULL)' without
`-Wall -Werror' produces a binary that SIGSEGVs
* compiling with `-Wall -Werror' errors out the compilation

Given this, maybe in the case of `ptsname_r' it is enough to just drop
the specific test that tests with NULL, and leave the man-page
unchanged? While we are at it, we could also drop the test for NULL in
`ptsname_r'. How does that sound for a PATCH v2?
  

On 05/25/2017 06:05 PM, Arjun Shankar wrote:

> Given this, maybe in the case of `ptsname_r' it is enough to just drop
> the specific test that tests with NULL, and leave the man-page
> unchanged?

That's fine as well.  I don't know if others want to preserve the EINVAL
handling.

> While we are at it, we could also drop the test for NULL in
> `ptsname_r'. How does that sound for a PATCH v2?

That would require updating the manpage, wouldn't it?

Thanks,
Florian
  

> > Given this, maybe in the case of `ptsname_r' it is enough to just drop
> > the specific test that tests with NULL, and leave the man-page
> > unchanged?
> 
> That's fine as well.  I don't know if others want to preserve the EINVAL
> handling.
> 
> > While we are at it, we could also drop the test for NULL in
> > `ptsname_r'. How does that sound for a PATCH v2?
> 
> That would require updating the manpage, wouldn't it?

My argument behind both of these suggestions was that `pthread_create'
doesn't care about both of these things (i.e. it doesn't check that the
`pthread_t' pointer is not NULL; it just writes to it. And it doesn't
explicitly mention in the man-page that the pointer is required to be
valid and of sufficient size) - and therefore `ptsname_r' doesn't
require these things either.

Am I reasoning about this correctly?
  

On Thu, May 25, 2017 at 11:37 PM, Arjun Shankar <arjun.is@lostca.se> wrote:
> My argument behind both of these suggestions was that `pthread_create'
> doesn't care about both of these things (i.e. it doesn't check that the
> `pthread_t' pointer is not NULL; it just writes to it. And it doesn't
> explicitly mention in the man-page that the pointer is required to be
> valid and of sufficient size) - and therefore `ptsname_r' doesn't
> require these things either.
>
> Am I reasoning about this correctly?

What you are saying is not wrong, but the manual page in its current
state documents the EINVAL return value.  That would have to change.

Thanks,
Florian