| Message ID | 20230710220659.3501429-2-xry111@xry111.site |
|---|---|
| State | New |
| Headers | show |
| Series | Revert "libio: Add __nonnull for FILE * arguments of fclose and freopen" | expand |
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
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
* 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: > * 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
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 --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
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(-)