diff mbox series

Revert "libio: Add __nonnull for FILE * arguments of fclose and freopen"

Message ID 20230710220659.3501429-2-xry111@xry111.site
State Superseded
Headers
Series Revert "libio: Add __nonnull for FILE * arguments of fclose and freopen" |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Testing passed

Commit Message

Xi Ruoyao July 10, 2023, 10:07 p.m. UTC 
  This reverts commit 71d9e0fe766a3c22a730995b9d024960970670af.

Apparantly the maintainers do not like __nonnull.  And I'm too pissed
off to work on this anymore.  Anyway I don't care about the analyzer so
they can just add these as ugly special analyzer patterns.  And I'm not
so stupid to pass NULL to these things myself, so lacking a warning is
not a problem to me.

Signed-off-by: Xi Ruoyao <xry111@xry111.site>
---
 libio/stdio.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Siddhesh Poyarekar July 10, 2023, 10:34 p.m. UTC | #1 
On 2023-07-10 18:07, Xi Ruoyao wrote:
> This reverts commit 71d9e0fe766a3c22a730995b9d024960970670af.
> 
> Apparantly the maintainers do not like __nonnull.  And I'm too pissed
> off to work on this anymore.  Anyway I don't care about the analyzer so
> they can just add these as ugly special analyzer patterns.  And I'm not
> so stupid to pass NULL to these things myself, so lacking a warning is
> not a problem to me.

Sorry you feel this way, but this is still unresolved as we don't have a 
consensus yet.  However I understand if you're frustrated and don't want 
to work on this for now; I do hope you return though.

In any case, if the consensus does steer towards never using 
__nonnull__, it'll likely be better to do it by hacking cdefs.h to 
expand __nonnull to nothing.

Thanks,
Sid

> 
> Signed-off-by: Xi Ruoyao <xry111@xry111.site>
> ---
>   libio/stdio.h | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libio/stdio.h b/libio/stdio.h
> index 4cf9f1c012..2387590d6a 100644
> --- a/libio/stdio.h
> +++ b/libio/stdio.h
> @@ -180,7 +180,7 @@ extern int renameat2 (int __oldfd, const char *__old, int __newfd,
>   
>      This function is a possible cancellation point and therefore not
>      marked with __THROW.  */
> -extern int fclose (FILE *__stream) __nonnull ((1));
> +extern int fclose (FILE *__stream);
>   
>   #undef __attr_dealloc_fclose
>   #define __attr_dealloc_fclose __attr_dealloc (fclose, 1)
> @@ -269,7 +269,7 @@ extern FILE *fopen (const char *__restrict __filename,
>      marked with __THROW.  */
>   extern FILE *freopen (const char *__restrict __filename,
>   		      const char *__restrict __modes,
> -		      FILE *__restrict __stream) __wur __nonnull ((3));
> +		      FILE *__restrict __stream) __wur;
>   #else
>   # ifdef __REDIRECT
>   extern FILE *__REDIRECT (fopen, (const char *__restrict __filename,
> @@ -290,7 +290,7 @@ extern FILE *fopen64 (const char *__restrict __filename,
>     __attribute_malloc__ __attr_dealloc_fclose __wur;
>   extern FILE *freopen64 (const char *__restrict __filename,
>   			const char *__restrict __modes,
> -			FILE *__restrict __stream) __wur __nonnull ((3));
> +			FILE *__restrict __stream) __wur;
>   #endif
>   
>   #ifdef	__USE_POSIX
Sam James July 11, 2023, 12:31 a.m. UTC | #2 
Siddhesh Poyarekar <siddhesh@gotplt.org> writes:

> On 2023-07-10 18:07, Xi Ruoyao wrote:
>> This reverts commit 71d9e0fe766a3c22a730995b9d024960970670af.
>> Apparantly the maintainers do not like __nonnull.  And I'm too
>> pissed
>> off to work on this anymore.  Anyway I don't care about the analyzer so
>> they can just add these as ugly special analyzer patterns.  And I'm not
>> so stupid to pass NULL to these things myself, so lacking a warning is
>> not a problem to me.
>
> Sorry you feel this way, but this is still unresolved as we don't have
> a consensus yet.  However I understand if you're frustrated and don't
> want to work on this for now; I do hope you return though.
>
> In any case, if the consensus does steer towards never using
> __nonnull__, it'll likely be better to do it by hacking cdefs.h to
> expand __nonnull to nothing.

Right. We'd need to cull it entirely (and I'm not convinced that's
the right thing to do). Xi Ruoyao's filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110617
too, to which I'm going to add some context in a moment.

>
> Thanks,
> Sid
>
>> Signed-off-by: Xi Ruoyao <xry111@xry111.site>
>> ---
>>   libio/stdio.h | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>> diff --git a/libio/stdio.h b/libio/stdio.h
>> index 4cf9f1c012..2387590d6a 100644
>> --- a/libio/stdio.h
>> +++ b/libio/stdio.h
>> @@ -180,7 +180,7 @@ extern int renameat2 (int __oldfd, const char *__old, int __newfd,
>>        This function is a possible cancellation point and therefore
>> not
>>      marked with __THROW.  */
>> -extern int fclose (FILE *__stream) __nonnull ((1));
>> +extern int fclose (FILE *__stream);
>>     #undef __attr_dealloc_fclose
>>   #define __attr_dealloc_fclose __attr_dealloc (fclose, 1)
>> @@ -269,7 +269,7 @@ extern FILE *fopen (const char *__restrict __filename,
>>      marked with __THROW.  */
>>   extern FILE *freopen (const char *__restrict __filename,
>>   		      const char *__restrict __modes,
>> -		      FILE *__restrict __stream) __wur __nonnull ((3));
>> +		      FILE *__restrict __stream) __wur;
>>   #else
>>   # ifdef __REDIRECT
>>   extern FILE *__REDIRECT (fopen, (const char *__restrict __filename,
>> @@ -290,7 +290,7 @@ extern FILE *fopen64 (const char *__restrict __filename,
>>     __attribute_malloc__ __attr_dealloc_fclose __wur;
>>   extern FILE *freopen64 (const char *__restrict __filename,
>>   			const char *__restrict __modes,
>> -			FILE *__restrict __stream) __wur __nonnull ((3));
>> +			FILE *__restrict __stream) __wur;
>>   #endif
>>     #ifdef	__USE_POSIX
Florian Weimer July 11, 2023, 5:15 a.m. UTC | #3 
* Siddhesh Poyarekar:

> On 2023-07-10 18:07, Xi Ruoyao wrote:
>> This reverts commit 71d9e0fe766a3c22a730995b9d024960970670af.
>> Apparantly the maintainers do not like __nonnull.  And I'm too
>> pissed
>> off to work on this anymore.  Anyway I don't care about the analyzer so
>> they can just add these as ugly special analyzer patterns.  And I'm not
>> so stupid to pass NULL to these things myself, so lacking a warning is
>> not a problem to me.
>
> Sorry you feel this way, but this is still unresolved as we don't have
> a consensus yet.  However I understand if you're frustrated and don't
> want to work on this for now; I do hope you return though.
>
> In any case, if the consensus does steer towards never using
> __nonnull__, it'll likely be better to do it by hacking cdefs.h to
> expand __nonnull to nothing.

We can do this under #ifdef _LIBC.  This way, analyzers can still
benefit from the annotations, but the attribute does not affect the
glibc build.

It doesn't make sense to maintain the nonnull-ness of glibc function
arguments outside glibc, so we should add these annotations to the
installed headers.

Thanks,
Florian
Florian Weimer July 11, 2023, 8:57 a.m. UTC | #4 
* Florian Weimer:

> * Siddhesh Poyarekar:
>
>> On 2023-07-10 18:07, Xi Ruoyao wrote:
>>> This reverts commit 71d9e0fe766a3c22a730995b9d024960970670af.
>>> Apparantly the maintainers do not like __nonnull.  And I'm too
>>> pissed
>>> off to work on this anymore.  Anyway I don't care about the analyzer so
>>> they can just add these as ugly special analyzer patterns.  And I'm not
>>> so stupid to pass NULL to these things myself, so lacking a warning is
>>> not a problem to me.
>>
>> Sorry you feel this way, but this is still unresolved as we don't have
>> a consensus yet.  However I understand if you're frustrated and don't
>> want to work on this for now; I do hope you return though.
>>
>> In any case, if the consensus does steer towards never using
>> __nonnull__, it'll likely be better to do it by hacking cdefs.h to
>> expand __nonnull to nothing.
>
> We can do this under #ifdef _LIBC.  This way, analyzers can still
> benefit from the annotations, but the attribute does not affect the
> glibc build.
>
> It doesn't make sense to maintain the nonnull-ness of glibc function
> arguments outside glibc, so we should add these annotations to the
> installed headers.

While trying to implement that, I found this in include/sys/cdefs.h:

/* The compiler will optimize based on the knowledge the parameter is
   not NULL.  This will omit tests.  A robust implementation cannot allow
   this so when compiling glibc itself we ignore this attribute.  */
# undef __nonnull
# define __nonnull(params)

So the whole thing is really a non-issue, and I don't think we need to
revert anything.

We can stop doing that #undef dance if there's an -f flag to get the GCC
behavior we want, so that we benefit from GCC diagnostics during the
glibc build.

Thanks,
Florian
Siddhesh Poyarekar July 11, 2023, 11:20 a.m. UTC | #5 
On 2023-07-11 04:57, Florian Weimer wrote:
> * Florian Weimer:
> 
>> * Siddhesh Poyarekar:
>>
>>> On 2023-07-10 18:07, Xi Ruoyao wrote:
>>>> This reverts commit 71d9e0fe766a3c22a730995b9d024960970670af.
>>>> Apparantly the maintainers do not like __nonnull.  And I'm too
>>>> pissed
>>>> off to work on this anymore.  Anyway I don't care about the analyzer so
>>>> they can just add these as ugly special analyzer patterns.  And I'm not
>>>> so stupid to pass NULL to these things myself, so lacking a warning is
>>>> not a problem to me.
>>>
>>> Sorry you feel this way, but this is still unresolved as we don't have
>>> a consensus yet.  However I understand if you're frustrated and don't
>>> want to work on this for now; I do hope you return though.
>>>
>>> In any case, if the consensus does steer towards never using
>>> __nonnull__, it'll likely be better to do it by hacking cdefs.h to
>>> expand __nonnull to nothing.
>>
>> We can do this under #ifdef _LIBC.  This way, analyzers can still
>> benefit from the annotations, but the attribute does not affect the
>> glibc build.
>>
>> It doesn't make sense to maintain the nonnull-ness of glibc function
>> arguments outside glibc, so we should add these annotations to the
>> installed headers.
> 
> While trying to implement that, I found this in include/sys/cdefs.h:
> 
> /* The compiler will optimize based on the knowledge the parameter is
>     not NULL.  This will omit tests.  A robust implementation cannot allow
>     this so when compiling glibc itself we ignore this attribute.  */
> # undef __nonnull
> # define __nonnull(params)

Right, I mentioned this during the review of Xi's earlier patch.  The 
side-effect of __nonnull is only limited to the caller.

Sid
diff mbox series

Patch

diff --git a/libio/stdio.h b/libio/stdio.h
index 4cf9f1c012..2387590d6a 100644
--- a/libio/stdio.h
+++ b/libio/stdio.h
@@ -180,7 +180,7 @@  extern int renameat2 (int __oldfd, const char *__old, int __newfd,
 
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
-extern int fclose (FILE *__stream) __nonnull ((1));
+extern int fclose (FILE *__stream);
 
 #undef __attr_dealloc_fclose
 #define __attr_dealloc_fclose __attr_dealloc (fclose, 1)
@@ -269,7 +269,7 @@  extern FILE *fopen (const char *__restrict __filename,
    marked with __THROW.  */
 extern FILE *freopen (const char *__restrict __filename,
 		      const char *__restrict __modes,
-		      FILE *__restrict __stream) __wur __nonnull ((3));
+		      FILE *__restrict __stream) __wur;
 #else
 # ifdef __REDIRECT
 extern FILE *__REDIRECT (fopen, (const char *__restrict __filename,
@@ -290,7 +290,7 @@  extern FILE *fopen64 (const char *__restrict __filename,
   __attribute_malloc__ __attr_dealloc_fclose __wur;
 extern FILE *freopen64 (const char *__restrict __filename,
 			const char *__restrict __modes,
-			FILE *__restrict __stream) __wur __nonnull ((3));
+			FILE *__restrict __stream) __wur;
 #endif
 
 #ifdef	__USE_POSIX