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

[Adhemerval Zanella Netto]

On 12/04/23 12:56, Jonny Grant wrote:
> 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
> 

I would suggest to track https://sourceware.org/git/?p=glibc.git, it is the
official glibc repo and the used for development.  And usually I check for
such changes using the normal git tools (blame, history, etc.), and 
occasionally with some graphical tool to get a more comprehensible history
view (gitk, etc.).

> 
>>> 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?

It reasonable to expect a non-null argument for glob, however glibc will
need to carry a similar hack from gnulib to keep the test for compatibility
(so program built against old header does not trigger invalid memory header).

> 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

This does not seem to be a good example to not use the nonnull attribute,
since from the brief discussion that nonnull attribute was added later
after API was initially set.  It means that if you have callers that rely
on the null check, if just add the nonnull attribute and remove the check
you are essentially breaking the API without either suppressing the attribute
on function implementation or providing a compatibility symbol (which is not
even widely supported on ELF world).

And as Xi Ruoyao has added, if your define your API to accept null argument
and use nonnull attribute this is an bug.

> 
> 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