* [PATCH] Revert "libio: Add __nonnull for FILE * arguments of fclose and freopen"
@ 2023-07-10 22:07 Xi Ruoyao
2023-07-10 22:34 ` Siddhesh Poyarekar
0 siblings, 1 reply; 6+ messages in thread
From: Xi Ruoyao @ 2023-07-10 22:07 UTC (permalink / raw)
To: libc-alpha
Cc: Adhemerval Zanella Netto, Carlos O'Donell, Alex Colomar,
Andreas Schwab, Siddhesh Poyarekar, Zack Weinberg, Jeff Law,
Xi Ruoyao
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(-)
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
--
2.41.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "libio: Add __nonnull for FILE * arguments of fclose and freopen"
2023-07-10 22:07 [PATCH] Revert "libio: Add __nonnull for FILE * arguments of fclose and freopen" Xi Ruoyao
@ 2023-07-10 22:34 ` Siddhesh Poyarekar
2023-07-11 0:31 ` Sam James
2023-07-11 5:15 ` Florian Weimer
0 siblings, 2 replies; 6+ messages in thread
From: Siddhesh Poyarekar @ 2023-07-10 22:34 UTC (permalink / raw)
To: Xi Ruoyao, libc-alpha
Cc: Adhemerval Zanella Netto, Carlos O'Donell, Alex Colomar,
Andreas Schwab, Zack Weinberg, Jeff Law
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "libio: Add __nonnull for FILE * arguments of fclose and freopen"
2023-07-10 22:34 ` Siddhesh Poyarekar
@ 2023-07-11 0:31 ` Sam James
2023-07-11 5:15 ` Florian Weimer
1 sibling, 0 replies; 6+ messages in thread
From: Sam James @ 2023-07-11 0:31 UTC (permalink / raw)
To: Siddhesh Poyarekar
Cc: Xi Ruoyao, Adhemerval Zanella Netto, Carlos O'Donell,
Alex Colomar, Andreas Schwab, Zack Weinberg, Jeff Law,
libc-alpha
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "libio: Add __nonnull for FILE * arguments of fclose and freopen"
2023-07-10 22:34 ` Siddhesh Poyarekar
2023-07-11 0:31 ` Sam James
@ 2023-07-11 5:15 ` Florian Weimer
2023-07-11 8:57 ` Florian Weimer
1 sibling, 1 reply; 6+ messages in thread
From: Florian Weimer @ 2023-07-11 5:15 UTC (permalink / raw)
To: Siddhesh Poyarekar
Cc: Xi Ruoyao, libc-alpha, Adhemerval Zanella Netto,
Carlos O'Donell, Alex Colomar, Andreas Schwab, Zack Weinberg,
Jeff Law
* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "libio: Add __nonnull for FILE * arguments of fclose and freopen"
2023-07-11 5:15 ` Florian Weimer
@ 2023-07-11 8:57 ` Florian Weimer
2023-07-11 11:20 ` Siddhesh Poyarekar
0 siblings, 1 reply; 6+ messages in thread
From: Florian Weimer @ 2023-07-11 8:57 UTC (permalink / raw)
To: Siddhesh Poyarekar
Cc: Xi Ruoyao, libc-alpha, Adhemerval Zanella Netto,
Carlos O'Donell, Alex Colomar, Andreas Schwab, Zack Weinberg,
Jeff Law
* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "libio: Add __nonnull for FILE * arguments of fclose and freopen"
2023-07-11 8:57 ` Florian Weimer
@ 2023-07-11 11:20 ` Siddhesh Poyarekar
0 siblings, 0 replies; 6+ messages in thread
From: Siddhesh Poyarekar @ 2023-07-11 11:20 UTC (permalink / raw)
To: Florian Weimer
Cc: Xi Ruoyao, libc-alpha, Adhemerval Zanella Netto,
Carlos O'Donell, Alex Colomar, Andreas Schwab, Zack Weinberg,
Jeff Law
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-07-11 11:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-10 22:07 [PATCH] Revert "libio: Add __nonnull for FILE * arguments of fclose and freopen" Xi Ruoyao
2023-07-10 22:34 ` Siddhesh Poyarekar
2023-07-11 0:31 ` Sam James
2023-07-11 5:15 ` Florian Weimer
2023-07-11 8:57 ` Florian Weimer
2023-07-11 11:20 ` Siddhesh Poyarekar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).