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

  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
  

* 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
  

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
>
  

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.
  

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