[RFC v1 0/2] Use -fno-delete-null-pointer-checks to build glibc

[RFC v1 0/2] Use -fno-delete-null-pointer-checks to build glibc

[Alejandro Colomar]

Hi,

Glibc currently undefines __nonnull internally, to avoid compiler
optimizations that would make glibc unsafe.  The attribute is only
present in installed headers, but for internal code an #undef is used.

Xi is trying to improve the consistency in uses of __nonnull in the
installed headers, to avoid special cases in GCC's fanalyzer.  That
prompted the question of whether the attribute is safe, as the compiler
is known to optimize too much based on it.

Florian reported that there's -fno-delete-null-pointer-checks to tell
the compiler to not optimize, and only warn about incorrect uses of null
pointers.

Another idea arised from that: it would be good if glibc used __nonnull
internally to get more warnings.  We only need to take care to prevent
the compiler optimizations.

While I was trying to do that change, which seemed trivial (see this
patch set), I uncovered what seems some circular dependencies in the
glibc header files (see the build log below).  Could you please have a
look at it?

Thanks,
Alex

---- (re)Build log ----

$ make
make -r PARALLELMFLAGS="" -C ../nonnull objdir=`pwd` all
make[1]: Entering directory '/home/alx/src/gnu/glibc/nonnull'

type "make help" for help with common glibc makefile targets

make  subdir=csu -C csu ..=../ subdir_lib
make  subdir=iconv -C iconv ..=../ subdir_lib
make[2]: Entering directory '/home/alx/src/gnu/glibc/nonnull/iconv'
gcc gconv_conf.c -c -std=gnu11 -fgnu89-inline -fno-delete-null-pointer-checks  -g -O2 -Wall -Wwrite-strings -Wundef -Werror -fmerge-all-constants -frounding-math -fno-stack-protector -fno-common -Wp,-U_FORTIFY_SOURCE -Wstrict-prototypes -Wold-style-definition -fmath-errno    -fPIE   -DGCONV_PATH='"/opt/gnu/glibc/nonnull/lib/gconv"'  -ftls-model=initial-exec     -I../include -I/home/alx/src/gnu/glibc/.tmp/iconv  -I/home/alx/src/gnu/glibc/.tmp  -I../sysdeps/unix/sysv/linux/x86_64/64  -I../sysdeps/unix/sysv/linux/x86_64  -I../sysdeps/unix/sysv/linux/x86/include -I../sysdeps/unix/sysv/linux/x86  -I../sysdeps/x86/nptl  -I../sysdeps/unix/sysv/linux/wordsize-64  -I../sysdeps/x86_64/nptl  -I../sysdeps/unix/sysv/linux/include -I../sysdeps/unix/sysv/linux  -I../sysdeps/nptl  -I../sysdeps/pthread  -I../sysdeps/gnu  -I../sysdeps/unix/inet  -I../sysdeps/unix/sysv  -I../sysdeps/unix/x86_64  -I../sysdeps/unix  -I../sysdeps/posix  -I../sysdeps/x86_64/64  -I../sysdeps/x86_64/fpu/multiarch  -I../sysdeps/x86_64/fpu  -I../sysdeps/x86/fpu  -I../sysdeps/x86_64/multiarch  -I../sysdeps/x86_64  -I../sysdeps/x86/include -I../sysdeps/x86  -I../sysdeps/ieee754/float128  -I../sysdeps/ieee754/ldbl-96/include -I../sysdeps/ieee754/ldbl-96  -I../sysdeps/ieee754/dbl-64  -I../sysdeps/ieee754/flt-32  -I../sysdeps/wordsize-64  -I../sysdeps/ieee754  -I../sysdeps/generic  -I.. -I../libio -I.  -D_LIBC_REENTRANT -include /home/alx/src/gnu/glibc/.tmp/libc-modules.h -DMODULE_NAME=libc -include ../include/libc-symbols.h  -DPIC     -DTOP_NAMESPACE=glibc -o /home/alx/src/gnu/glibc/.tmp/iconv/gconv_conf.o -MD -MP -MF /home/alx/src/gnu/glibc/.tmp/iconv/gconv_conf.o.dt -MT /home/alx/src/gnu/glibc/.tmp/iconv/gconv_conf.o
In file included from ../include/sys/cdefs.h:10,
                 from ../include/features.h:503,
                 from ../assert/assert.h:35,
                 from ../include/assert.h:1,
                 from gconv_conf.c:19:
../include/dirent.h:53:17: error: expected ‘)’ before numeric constant
   53 |      __nonnull (4) attribute_hidden;
      |                 ^
../misc/sys/cdefs.h:401:69: note: in definition of macro ‘__attribute_nonnull__’
  401 | #  define __attribute_nonnull__(params) __attribute__ ((__nonnull__ params))
      |                                                                     ^~~~~~
../include/dirent.h:53:6: note: in expansion of macro ‘__nonnull’
   53 |      __nonnull (4) attribute_hidden;
      |      ^~~~~~~~~
../misc/sys/cdefs.h:401:76: error: expected ‘,’ or ‘;’ before ‘)’ token
  401 | #  define __attribute_nonnull__(params) __attribute__ ((__nonnull__ params))
      |                                                                            ^
../misc/sys/cdefs.h:407:28: note: in expansion of macro ‘__attribute_nonnull__’
  407 | # define __nonnull(params) __attribute_nonnull__ (params)
      |                            ^~~~~~~~~~~~~~~~~~~~~
../include/dirent.h:53:6: note: in expansion of macro ‘__nonnull’
   53 |      __nonnull (4) attribute_hidden;
      |      ^~~~~~~~~
make[2]: *** [../o-iterator.mk:9: /home/alx/src/gnu/glibc/.tmp/iconv/gconv_conf.o] Error 1
make[2]: Leaving directory '/home/alx/src/gnu/glibc/nonnull/iconv'
make[1]: *** [Makefile:484: iconv/subdir_lib] Error 2
make[1]: Leaving directory '/home/alx/src/gnu/glibc/nonnull'
make: *** [Makefile:9: all] Error 2

--- Build log end ----

Alejandro Colomar (2):
  Makeconfig: Use one line per token
  Makeconfig: Compile glibc with -fno-delete-null-pointer-checks

 Makeconfig          | 18 +++++++++++++-----
 include/sys/cdefs.h |  6 ------
 2 files changed, 13 insertions(+), 11 deletions(-)

Range-diff against v0:
-:  ---------- > 1:  55d7caa944 Makeconfig: Use one line per token
-:  ---------- > 2:  efb21da36c Makeconfig: Compile glibc with -fno-delete-null-pointer-checks
-- 
2.40.1

[RFC v1 1/2] Makeconfig: Use one line per token

[Alejandro Colomar]

After this change, patches will be more clear about what they change, as
they won't need to reflow lines.

Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 Makeconfig | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/Makeconfig b/Makeconfig
index 77d7fd14df..369a596e79 100644
--- a/Makeconfig
+++ b/Makeconfig
@@ -1043,11 +1043,18 @@ CPPFLAGS = $(config-extra-cppflags) $(CPPFLAGS-config) \
 	   $(CPPFLAGS-$(<F)) $(CPPFLAGS-$(@F)) $(CPPFLAGS-$(basename $(@F))) \
 	   -DTOP_NAMESPACE=glibc
 
-override CFLAGS	= -std=gnu11 -fgnu89-inline $(config-extra-cflags) \
-		  $(filter-out %frame-pointer,$(+cflags)) $(+gccwarn-c) \
-		  $(+extra-math-flags) $(+extra-time-flags) \
-		  $(sysdep-CFLAGS) $(CFLAGS-$(suffix $@)) $(CFLAGS-$(<F)) \
-		  $(CFLAGS-$(@F)) $(tls-model) \
+override CFLAGS	= -std=gnu11 \
+		  -fgnu89-inline \
+		  $(config-extra-cflags) \
+		  $(filter-out %frame-pointer,$(+cflags)) \
+		  $(+gccwarn-c) \
+		  $(+extra-math-flags) \
+		  $(+extra-time-flags) \
+		  $(sysdep-CFLAGS) \
+		  $(CFLAGS-$(suffix $@)) \
+		  $(CFLAGS-$(<F)) \
+		  $(CFLAGS-$(@F)) \
+		  $(tls-model) \
 		  $(foreach lib,$(libof-$(basename $(@F))) \
 				$(libof-$(<F)) $(libof-$(@F)),$(CFLAGS-$(lib)))
 # Use our copies of cstdlib and cmath.
-- 
2.40.1

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

[Alejandro Colomar]

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

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

Re: [RFC v1 0/2] Use -fno-delete-null-pointer-checks to build glibc

[Florian Weimer]

* Alejandro Colomar via Libc-alpha:

> In file included from ../include/sys/cdefs.h:10,
>                  from ../include/features.h:503,
>                  from ../assert/assert.h:35,
>                  from ../include/assert.h:1,
>                  from gconv_conf.c:19:
> ../include/dirent.h:53:17: error: expected ‘)’ before numeric constant
>    53 |      __nonnull (4) attribute_hidden;
>       |                 ^

This is a typo, it should be __nonnull ((4)).

The __nonnull redefinition simply hides it for now.

Thanks,
Florian

Re: [RFC v1 0/2] Use -fno-delete-null-pointer-checks to build glibc

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 786 bytes --]

On 2023-07-11 15:18, Florian Weimer wrote:
> * Alejandro Colomar via Libc-alpha:
> 
>> In file included from ../include/sys/cdefs.h:10,
>>                  from ../include/features.h:503,
>>                  from ../assert/assert.h:35,
>>                  from ../include/assert.h:1,
>>                  from gconv_conf.c:19:
>> ../include/dirent.h:53:17: error: expected ‘)’ before numeric constant
>>    53 |      __nonnull (4) attribute_hidden;
>>       |                 ^
> 
> This is a typo, it should be __nonnull ((4)).
> 
> The __nonnull redefinition simply hides it for now.

Thanks.  I'll fix it.

Cheers,
Alex

> 
> Thanks,
> Florian
> 

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC v1 0/2] Use -fno-delete-null-pointer-checks to build glibc

[Xi Ruoyao]

On Tue, 2023-07-11 at 15:11 +0200, Alejandro Colomar wrote:
> Florian reported that there's -fno-delete-null-pointer-checks to tell
> the compiler to not optimize, and only warn about incorrect uses of null
> pointers.

One potential problem is -fno-delete-null-pointer-checks not only
disables optimizations based on __nonnull, but also disables the
optimizations based on other provable non-null pointer values.  So maybe
we should run a benchmark to see if there is some severe degradation, if
there is any we may fix it by moving null checks away from hot paths. 

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [RFC v1 0/2] Use -fno-delete-null-pointer-checks to build glibc

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 939 bytes --]

On 2023-07-11 15:22, Xi Ruoyao wrote:
> On Tue, 2023-07-11 at 15:11 +0200, Alejandro Colomar wrote:
>> Florian reported that there's -fno-delete-null-pointer-checks to tell
>> the compiler to not optimize, and only warn about incorrect uses of null
>> pointers.
> 
> One potential problem is -fno-delete-null-pointer-checks not only
> disables optimizations based on __nonnull, but also disables the
> optimizations based on other provable non-null pointer values.  So maybe
> we should run a benchmark to see if there is some severe degradation, if
> there is any we may fix it by moving null checks away from hot paths. 
> 

Sure; I don't know how to run those, so I'll provide the patches, and
will let any of you run the benchmarks (or point me to some documentation
to learn how to run them).

Cheers,
Alex

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Florian Weimer]

* 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

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

[Siddhesh Poyarekar]

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

Re: [RFC v1 1/2] Makeconfig: Use one line per token

[Adhemerval Zanella Netto]

On 11/07/23 10:11, Alejandro Colomar via Libc-alpha wrote:
> After this change, patches will be more clear about what they change, as
> they won't need to reflow lines.
> 
> Signed-off-by: Alejandro Colomar <alx@kernel.org>

I think this can be installed independently of the rest of the patchset.

> ---
>  Makeconfig | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/Makeconfig b/Makeconfig
> index 77d7fd14df..369a596e79 100644
> --- a/Makeconfig
> +++ b/Makeconfig
> @@ -1043,11 +1043,18 @@ CPPFLAGS = $(config-extra-cppflags) $(CPPFLAGS-config) \
>  	   $(CPPFLAGS-$(<F)) $(CPPFLAGS-$(@F)) $(CPPFLAGS-$(basename $(@F))) \
>  	   -DTOP_NAMESPACE=glibc
>  
> -override CFLAGS	= -std=gnu11 -fgnu89-inline $(config-extra-cflags) \
> -		  $(filter-out %frame-pointer,$(+cflags)) $(+gccwarn-c) \
> -		  $(+extra-math-flags) $(+extra-time-flags) \
> -		  $(sysdep-CFLAGS) $(CFLAGS-$(suffix $@)) $(CFLAGS-$(<F)) \
> -		  $(CFLAGS-$(@F)) $(tls-model) \
> +override CFLAGS	= -std=gnu11 \

Maybe move '-std=gnu11' to next line as well.

> +		  -fgnu89-inline \
> +		  $(config-extra-cflags) \
> +		  $(filter-out %frame-pointer,$(+cflags)) \
> +		  $(+gccwarn-c) \
> +		  $(+extra-math-flags) \
> +		  $(+extra-time-flags) \
> +		  $(sysdep-CFLAGS) \
> +		  $(CFLAGS-$(suffix $@)) \
> +		  $(CFLAGS-$(<F)) \
> +		  $(CFLAGS-$(@F)) \
> +		  $(tls-model) \
>  		  $(foreach lib,$(libof-$(basename $(@F))) \
>  				$(libof-$(<F)) $(libof-$(@F)),$(CFLAGS-$(lib)))
>  # Use our copies of cstdlib and cmath.

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

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

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

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)

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

[Cristian Rodríguez]

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]

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 ?

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

[Florian Weimer]

* 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