[RFC,v1,2/2] Makeconfig: Compile glibc with -fno-delete-null-pointer-checks

  Don't undefine __nonnull to prevent compiler optimizations.  Instead,
tell the compiler to not optimize based on the attribute, via the
appropriate flag.

Link: <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110617>
Reported-by: Florian Weimer <fweimer@redhat.com>
Cc: Xi Ruoyao <xry111@xry111.site>
Cc: Sam James <sam@gentoo.org>
Cc: Richard Biener <rguenth@gcc.gnu.org>
Cc: Andrew Pinski <apinski@marvell.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 Makeconfig          | 1 +
 include/sys/cdefs.h | 6 ------
 2 files changed, 1 insertion(+), 6 deletions(-)
  

* Alejandro Colomar via Libc-alpha:

> Don't undefine __nonnull to prevent compiler optimizations.  Instead,
> tell the compiler to not optimize based on the attribute, via the
> appropriate flag.

The manual for GCC 13 says:

     For function definitions:
        • If the compiler determines that a function parameter that is
          marked with nonnull is compared with null, and
          ‘-Wnonnull-compare’ option is enabled, a warning is issued.
          *Note Warning Options::.
        • The compiler may also perform optimizations based on the
          knowledge that ‘nonnull’ parameters cannot be null.  This can
          currently not be disabled other than by removing the nonnull
          attribute.

So this isn't actually equivalent today, and I think it impacts our
ability to deal with null pointer errors in the implementation (in
function definitions).

Thanks,
Florian
  

On 2023-07-11 09:41, Florian Weimer via Libc-alpha wrote:
> * Alejandro Colomar via Libc-alpha:
> 
>> Don't undefine __nonnull to prevent compiler optimizations.  Instead,
>> tell the compiler to not optimize based on the attribute, via the
>> appropriate flag.
> 
> The manual for GCC 13 says:
> 
>       For function definitions:
>          • If the compiler determines that a function parameter that is
>            marked with nonnull is compared with null, and
>            ‘-Wnonnull-compare’ option is enabled, a warning is issued.
>            *Note Warning Options::.
>          • The compiler may also perform optimizations based on the
>            knowledge that ‘nonnull’ parameters cannot be null.  This can
>            currently not be disabled other than by removing the nonnull
>            attribute.
> 
> So this isn't actually equivalent today, and I think it impacts our
> ability to deal with null pointer errors in the implementation (in
> function definitions).

+1, this situation is equivalent to why we remove the access attribute 
during fortification too; it hinders the ability of the implementation 
to do those checks at runtime.

Thanks,
Sid
  

On Tue, 11 Jul 2023, Alejandro Colomar wrote:

> Don't undefine __nonnull to prevent compiler optimizations.  Instead,
> tell the compiler to not optimize based on the attribute, via the
> appropriate flag.

I'll note that using -fno-delete-null-pointer-checks doesn't work
(anymore?) to preserve the nullptr check in

int __attribute__((nonnull)) foo (int *p) 
{
  if (p)
    return *p;
  return 0;
}

so the documentation of 'nonnull' which says

@item The compiler may also perform optimizations based on the
knowledge that certain function arguments cannot be null.  These
optimizations can be disabled by the
@option{-fno-delete-null-pointer-checks} option. @xref{Optimize Options}.

isn't accurate.

I checked trunk, GCC 12 and GCC 7 here.  For trunk the offending
function is nonnull_arg_p where we probably want to preserve
its behavior but some callers fail to check 
flag_delete_null_pointer_checks when they are not only issueing
diagnostics.

Richard.

> Link: <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110617>
> Reported-by: Florian Weimer <fweimer@redhat.com>
> Cc: Xi Ruoyao <xry111@xry111.site>
> Cc: Sam James <sam@gentoo.org>
> Cc: Richard Biener <rguenth@gcc.gnu.org>
> Cc: Andrew Pinski <apinski@marvell.com>
> Signed-off-by: Alejandro Colomar <alx@kernel.org>
> ---
>  Makeconfig          | 1 +
>  include/sys/cdefs.h | 6 ------
>  2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/Makeconfig b/Makeconfig
> index 369a596e79..ecbe30e53f 100644
> --- a/Makeconfig
> +++ b/Makeconfig
> @@ -1045,6 +1045,7 @@ CPPFLAGS = $(config-extra-cppflags) $(CPPFLAGS-config) \
>  
>  override CFLAGS	= -std=gnu11 \
>  		  -fgnu89-inline \
> +		  -fno-delete-null-pointer-checks \
>  		  $(config-extra-cflags) \
>  		  $(filter-out %frame-pointer,$(+cflags)) \
>  		  $(+gccwarn-c) \
> diff --git a/include/sys/cdefs.h b/include/sys/cdefs.h
> index b84ad34a70..e4a8dd7844 100644
> --- a/include/sys/cdefs.h
> +++ b/include/sys/cdefs.h
> @@ -10,12 +10,6 @@
>  #include <misc/sys/cdefs.h>
>  
>  #ifndef _ISOMAC
> -/* 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)
> -
>  extern void __chk_fail (void) __attribute__ ((__noreturn__));
>  libc_hidden_proto (__chk_fail)
>  rtld_hidden_proto (__chk_fail)
>
  

On Wed, Jul 12, 2023 at 6:56 AM Richard Biener via Libc-alpha <
libc-alpha@sourceware.org> wrote:

>
>
> I checked trunk, GCC 12 and GCC 7 here.  For trunk the offending
> function is nonnull_arg_p where we probably want to preserve
> its behavior but some callers fail to check
> flag_delete_null_pointer_checks when they are not only issueing
> diagnostics.
>
>
int __attribute__((nonnull)) foo (int p) does not error out either at least
in gcc-14.. if there is no pointer argument then the attribute is wrong
ins't it ?
  

* Richard Biener:

> On Tue, 11 Jul 2023, Alejandro Colomar wrote:
>
>> Don't undefine __nonnull to prevent compiler optimizations.  Instead,
>> tell the compiler to not optimize based on the attribute, via the
>> appropriate flag.
>
> I'll note that using -fno-delete-null-pointer-checks doesn't work
> (anymore?) to preserve the nullptr check in
>
> int __attribute__((nonnull)) foo (int *p) 
> {
>   if (p)
>     return *p;
>   return 0;
> }
>
> so the documentation of 'nonnull' which says
>
> @item The compiler may also perform optimizations based on the
> knowledge that certain function arguments cannot be null.  These
> optimizations can be disabled by the
> @option{-fno-delete-null-pointer-checks} option. @xref{Optimize Options}.
>
> isn't accurate.

That's the paragraph about function calls, not function definitions.
The above is a function definition.  So it's not a counterexample.  The
second paragraph says that the optimization behavior you see is
currently not controllable by an option.

Thanks,
Florian