Re: glibc misc/sys/cdefs.h nonull - typo in comment

[Jonny Grant <jg@jguk.org>]

Hi Adhemerval

On 11/04/2023 14:39, Adhemerval Zanella Netto wrote:
> 
> 
> On 11/04/23 06:09, Jonny Grant wrote:
>> Hi
>>
>> There are two small changes I suggest if someone has a moment?
>>
>> misc/sys/cdefs.h  nonull  - typo in comment
>> Should be "nonnull"
> 
> This is already being fixed by c8ba52ab3350c334d (2.34).

That's great news!

I was interested to have a look through the history, and see the changes.

Sorry I am bit new to sourceware git repo browser. 
May I ask, is there an easy way to look up that hex? It looks shorter than usual commit hex strings.

The way I found the change was to use https://sourceware.org/git/?p=glibc.git  "grep" search box with "c8ba52ab3350c334d" which found the ChangeLog with c8ba52ab3350c334d6e34b1439a4c0c1431351f3

Then I found another commit, and added to the URL
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=

https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=c8ba52ab3350c334d6e34b1439a4c0c1431351f3


>> Also
>>
>> /posix/glob.c has an unused macro
>> # define _GL_ARG_NONNULL(params)
>>
>> Could that be removed?
> 
> This definition is used by gnulib code, since it defined for !_LIBC,
> Different than glibc, gnulib defines the function as:
> 
> lib/glob.in.h
> 
> 103 #if @GNULIB_GLOB@
> 104 # if @REPLACE_GLOB@
> 105 _GL_FUNCDECL_RPL (glob, int, (const char *_Restrict_ __pattern, int __flags,
> 106                               _gl_glob_errfunc_fn __errfunc,
> 107                               glob_t *_Restrict_ __pglob)
> 108                               _GL_ARG_NONNULL ((1)));
> 109 _GL_CXXALIAS_RPL (glob, int, (const char *_Restrict_ __pattern, int __flags,
> 110                               _gl_glob_errfunc_fn __errfunc,
> 111                               glob_t *_Restrict_ __pglob));
> 112 # else
> 113 #  if !@HAVE_GLOB@
> 114 _GL_FUNCDECL_SYS (glob, int, (const char *_Restrict_ __pattern, int __flags,
> 115                               _gl_glob_errfunc_fn __errfunc,
> 116                               glob_t *_Restrict_ __pglob)
> 117                               _GL_ARG_NONNULL ((1)));
> 118 #  endif
> 119 _GL_CXXALIAS_SYS (glob, int, (const char *_Restrict_ __pattern, int __flags,
> 120                               _gl_glob_errfunc_fn __errfunc,
> 121                               glob_t *_Restrict_ __pglob));
> 122 # endif
> 
> Which then at the implementation is also check for pattern == NULL:
> 
> lib/glob.c:
> 
>  316   if (pattern == NULL || pglob == NULL || (flags & ~__GLOB_FLAGS) != 0)
>  317     {
>  318       __set_errno (EINVAL);
>  319       return -1;
>  320     }
> 
> So the comment is right that compiler might indeed remove the test. 
> Different than gnulib, glibc prototype does not add the 
> __attribute__ ((nonnul)).
> 
>>
>> Seems _GL_ARG_NONNULL is only used in misc/error.c which again just compiles it out. So maybe it can be removed overall in both files?
> 
> This was added as a sync with gnulib code by 888c679ba40, because it is
> used in error_tail definition and glibc does not define it.  So we can not
> remove without also adjusting error_tail, and since the code is originally
> from gnulib maybe it would be better to use __nonnull macro.

Thank you for your reply with information. I'll read up a bit more next time before asking!

Do you think it is risky to have the nonnull attribute in use in glibc?
Some projects have abandoned attribute nonnull due to the GCC optimizer using the nonnull attribute to remove the runtime null pointer checks.
https://gitlab.com/gnuwget/wget2/-/issues/200

Currently cdefs.h has

/* The nonnull function attribute marks pointer parameters that
   must not be NULL.  This has the name __nonnull in glibc,
   and __attribute_nonnull__ in files shared with Gnulib to avoid
   collision with a different __nonnull in DragonFlyBSD 5.9.  */

Is it better to clarify this to be something like the following?

/* The nonnull function attribute marks pointer parameters that
   the compiler's optimizer knows will never be NULL. This means NULL checks can be optimized out.
   This has the name __nonnull in glibc,
   and __attribute_nonnull__ in files shared with Gnulib to avoid
   collision with a different __nonnull in DragonFlyBSD 5.9.  */


Kind regards, Jonny