Re: [PATCH] Define NULL as __null in C++ mode when using GCC or Clang.

[Rich Felker <dalias@libc.org>]

On Tue, Jul 09, 2019 at 03:19:00PM -0400, James Y Knight wrote:
> Both GCC and Clang ship their own stddef.h which does this (musl's
> stddef.h is simply ignored). But, musl also defines the macro in a
> number of other headers.

This was intentional at the time it was done. At least in the present
version of C++ at the time, __null was not a conforming definition.
For example, conforming applications could parse the stringification
of NULL and expect to get something that's a valid null pointer
constant. This could possibly be revisited, if it's changed, and if it
can be done in a way that doesn't involve compiler-specific stuff.

> Thus, depending on which header you include
> last, you'll end up with a different definition of NULL.

musl doesn't support use of the compiler versions of these headers,
for several reasons that have nothing to do with NULL.

> Mostly, getting musl's definition simply degrades warning diagnostics
> in C++ slightly -- e.g. GCC can no longer emit this warning:
>   warning: passing NULL to non-pointer argument 1 of 'int foo(long int)'
> [-Wconversion-null]

The current definition however catches bugs where NULL is wrongly used
to terminate a variadic argument list where (void*)0 or (char*)0 is
actually needed. See commits 41d7c77d6a2e74294807d35062e4cd1d48ab72d3
and c8a9c22173f485c8c053709e1dfa0a617cb6be1a. So both choices are a
tradeoff in diagnostic capability.

> If you're using Clang's modules support, it can also break
> compilation. In that case, the conflicting definitions may be detected
> as a module incompatibility.

I'm not sure what this means. The conflicting definition should never
appear in code that's consistent with compiling and linking with musl.

> A different (potentially better) fix would be to always retrieve the
> definition of NULL from the compiler's stddef.h (via #define
> __need_NULL #include <stddef.h>). It may also be best to delete the
> musl stddef.h entirely for clarity, since it's currently ignored,
> anyhow.

We intentionally don't do things that way.

Rich


> But, this seemed the more minimal fix.

> From 3d898d4825c28f08ade92c40822fa5bfa2ef1f1f Mon Sep 17 00:00:00 2001
> From: James Y Knight <jyknight@google.com>
> Date: Tue, 9 Jul 2019 15:03:03 -0400
> Subject: [PATCH] Define NULL as __null in C++ mode when using GCC or Clang.
> 
> Both GCC and Clang ship their own stddef.h which does this (musl's
> stddef.h is simply ignored). But, musl also defines the macro in a
> number of other headers. Thus, depending on which header you include
> last, you'll end up with a different definition of NULL.
> 
> Mostly, getting musl's definition simply degrades warning diagnostics
> in C++ slightly -- e.g. GCC can no longer emit this warning:
>   warning: passing NULL to non-pointer argument 1 of 'int foo(long int)' [-Wconversion-null]
> 
> If you're using Clang's modules support, it can also break
> compilation. In that case, the conflicting definitions may be detected
> as a module incompatibility.
> 
> A different (potentially better) fix would be to always retrieve the
> definition of NULL from the compiler's stddef.h (via #define
> __need_NULL #include <stddef.h>). It may also be best to delete the
> musl stddef.h entirely for clarity, since it's currently ignored,
> anyhow.
> 
> But, this seemed the more minimal fix.
> ---
>  include/locale.h | 4 ++++
>  include/stddef.h | 4 ++++
>  include/stdio.h  | 4 ++++
>  include/stdlib.h | 4 ++++
>  include/string.h | 4 ++++
>  include/time.h   | 4 ++++
>  include/unistd.h | 4 ++++
>  include/wchar.h  | 4 ++++
>  8 files changed, 32 insertions(+)
> 
> diff --git a/include/locale.h b/include/locale.h
> index ce384381..80c2d2db 100644
> --- a/include/locale.h
> +++ b/include/locale.h
> @@ -8,7 +8,11 @@ extern "C" {
>  #include <features.h>
>  
>  #ifdef __cplusplus
> +#ifdef __GNUC__
> +#define NULL __null
> +#else
>  #define NULL 0L
> +#endif
>  #else
>  #define NULL ((void*)0)
>  #endif
> diff --git a/include/stddef.h b/include/stddef.h
> index bd753853..415a2d91 100644
> --- a/include/stddef.h
> +++ b/include/stddef.h
> @@ -2,7 +2,11 @@
>  #define _STDDEF_H
>  
>  #ifdef __cplusplus
> +#ifdef __GNUC__
> +#define NULL __null
> +#else
>  #define NULL 0L
> +#endif
>  #else
>  #define NULL ((void*)0)
>  #endif
> diff --git a/include/stdio.h b/include/stdio.h
> index 3604198c..9e30d1ad 100644
> --- a/include/stdio.h
> +++ b/include/stdio.h
> @@ -26,7 +26,11 @@ extern "C" {
>  #include <bits/alltypes.h>
>  
>  #ifdef __cplusplus
> +#ifdef __GNUC__
> +#define NULL __null
> +#else
>  #define NULL 0L
> +#endif
>  #else
>  #define NULL ((void*)0)
>  #endif
> diff --git a/include/stdlib.h b/include/stdlib.h
> index 42ca8336..a272a4f4 100644
> --- a/include/stdlib.h
> +++ b/include/stdlib.h
> @@ -8,7 +8,11 @@ extern "C" {
>  #include <features.h>
>  
>  #ifdef __cplusplus
> +#ifdef __GNUC__
> +#define NULL __null
> +#else
>  #define NULL 0L
> +#endif
>  #else
>  #define NULL ((void*)0)
>  #endif
> diff --git a/include/string.h b/include/string.h
> index 795a2abc..4d344d72 100644
> --- a/include/string.h
> +++ b/include/string.h
> @@ -8,7 +8,11 @@ extern "C" {
>  #include <features.h>
>  
>  #ifdef __cplusplus
> +#ifdef __GNUC__
> +#define NULL __null
> +#else
>  #define NULL 0L
> +#endif
>  #else
>  #define NULL ((void*)0)
>  #endif
> diff --git a/include/time.h b/include/time.h
> index 672b3fc3..0eec373c 100644
> --- a/include/time.h
> +++ b/include/time.h
> @@ -8,7 +8,11 @@ extern "C" {
>  #include <features.h>
>  
>  #ifdef __cplusplus
> +#ifdef __GNUC__
> +#define NULL __null
> +#else
>  #define NULL 0L
> +#endif
>  #else
>  #define NULL ((void*)0)
>  #endif
> diff --git a/include/unistd.h b/include/unistd.h
> index 9485da7a..391b58ba 100644
> --- a/include/unistd.h
> +++ b/include/unistd.h
> @@ -16,7 +16,11 @@ extern "C" {
>  #define SEEK_END 2
>  
>  #ifdef __cplusplus
> +#ifdef __GNUC__
> +#define NULL __null
> +#else
>  #define NULL 0L
> +#endif
>  #else
>  #define NULL ((void*)0)
>  #endif
> diff --git a/include/wchar.h b/include/wchar.h
> index 88eb55b1..bfdbaebb 100644
> --- a/include/wchar.h
> +++ b/include/wchar.h
> @@ -39,7 +39,11 @@ extern "C" {
>  #endif
>  
>  #ifdef __cplusplus
> +#ifdef __GNUC__
> +#define NULL __null
> +#else
>  #define NULL 0L
> +#endif
>  #else
>  #define NULL ((void*)0)
>  #endif
> -- 
> 2.22.0.410.gd8fdbe21b5-goog
>