[clang][headers] Including stddef.h always redefines NULL #99727

ian-twilightcoder · 2024-07-20T00:54:47Z

stddef.h always includes __stddef_null.h. This is fine in modules because it's not possible to re-include the pcm, and it's necessary to export the _Builtin_stddef.null submodule. However, without modules it causes NULL to always get redefined which disrupts some C++ code. Rework the inclusion of __stddef_null.h so that with not building with modules it's only included if __need_NULL is set by the includer, or it's the first time stddef.h is being included.

llvmbot · 2024-07-20T00:55:19Z

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-backend-x86

Author: Ian Anderson (ian-twilightcoder)

Changes

stddef.h always includes __stddef_null.h. This is fine in modules because it's not possible to re-include the pcm, and it's necessary to export the _Builtin_stddef.null submodule. However, without modules it causes NULL to always get redefined which disrupts some C++ code. Rework the inclusion of __stddef_null.h so that with not building with modules it's only included if __need_NULL is set by the includer, or it's the first time stddef.h is being included.

Full diff: https://github.com/llvm/llvm-project/pull/99727.diff

3 Files Affected:

(modified) clang/lib/Headers/stdarg.h (+2-2)
(modified) clang/lib/Headers/stddef.h (+20-2)
(modified) clang/test/Headers/stddefneeds.cpp (+11-4)

diff --git a/clang/lib/Headers/stdarg.h b/clang/lib/Headers/stdarg.h
index 8292ab907becf..6203d7a600a23 100644
--- a/clang/lib/Headers/stdarg.h
+++ b/clang/lib/Headers/stdarg.h
@@ -20,19 +20,18 @@
  * modules.
  */
 #if defined(__MVS__) && __has_include_next(<stdarg.h>)
-#include <__stdarg_header_macro.h>
 #undef __need___va_list
 #undef __need_va_list
 #undef __need_va_arg
 #undef __need___va_copy
 #undef __need_va_copy
+#include <__stdarg_header_macro.h>
 #include_next <stdarg.h>
 
 #else
 #if !defined(__need___va_list) && !defined(__need_va_list) &&                  \
     !defined(__need_va_arg) && !defined(__need___va_copy) &&                   \
     !defined(__need_va_copy)
-#include <__stdarg_header_macro.h>
 #define __need___va_list
 #define __need_va_list
 #define __need_va_arg
@@ -45,6 +44,7 @@
     !defined(__STRICT_ANSI__)
 #define __need_va_copy
 #endif
+#include <__stdarg_header_macro.h>
 #endif
 
 #ifdef __need___va_list
diff --git a/clang/lib/Headers/stddef.h b/clang/lib/Headers/stddef.h
index 8985c526e8fc5..dab464805ccea 100644
--- a/clang/lib/Headers/stddef.h
+++ b/clang/lib/Headers/stddef.h
@@ -20,7 +20,6 @@
  * modules.
  */
 #if defined(__MVS__) && __has_include_next(<stddef.h>)
-#include <__stddef_header_macro.h>
 #undef __need_ptrdiff_t
 #undef __need_size_t
 #undef __need_rsize_t
@@ -31,6 +30,7 @@
 #undef __need_max_align_t
 #undef __need_offsetof
 #undef __need_wint_t
+#include <__stddef_header_macro.h>
 #include_next <stddef.h>
 
 #else
@@ -40,7 +40,6 @@
     !defined(__need_NULL) && !defined(__need_nullptr_t) &&                     \
     !defined(__need_unreachable) && !defined(__need_max_align_t) &&            \
     !defined(__need_offsetof) && !defined(__need_wint_t)
-#include <__stddef_header_macro.h>
 #define __need_ptrdiff_t
 #define __need_size_t
 /* ISO9899:2011 7.20 (C11 Annex K): Define rsize_t if __STDC_WANT_LIB_EXT1__ is
@@ -49,7 +48,25 @@
 #define __need_rsize_t
 #endif
 #define __need_wchar_t
+#if !defined(__STDDEF_H) && !__building_module(_Builtin_stddef)
+/*
+ * __stddef_null.h is special when building without modules: if __need_NULL is
+ * set, then it will unconditionally redefine NULL. To avoid stepping on client
+ * definitions of NULL, __need_NULL should only be set the first time this
+ * header is included, that is when __STDDEF_H is not defined. However, when
+ * building with modules, this header is a textual header and needs to
+ * unconditionally include __stdef_null.h to support multiple submodules
+ * exporting _Builtin_stddef.null. Take module SM with submodules A and B, whose
+ * headers both include stddef.h When SM.A builds, __STDDEF_H will be defined.
+ * When SM.B builds, the definition from SM.A will leak when building without
+ * local submodule visibility. stddef.h wouldn't include any of its
+ * implementation headers, and SM.B wouldn't import any of the stddef modules,
+ * and SM.B's `export *` wouldn't export any stddef interfaces as expected. When
+ * building with modules, always include __stddef_null.h so that everything
+ * works as expected.
+ */
 #define __need_NULL
+#endif
 #if (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202311L) ||              \
     defined(__cplusplus)
 #define __need_nullptr_t
@@ -65,6 +82,7 @@
 /* wint_t is provided by <wchar.h> and not <stddef.h>. It's here
  * for compatibility, but must be explicitly requested. Therefore
  * __need_wint_t is intentionally not defined here. */
+#include <__stddef_header_macro.h>
 #endif
 
 #if defined(__need_ptrdiff_t)
diff --git a/clang/test/Headers/stddefneeds.cpp b/clang/test/Headers/stddefneeds.cpp
index 0763bbdee13ae..0282e8afa600d 100644
--- a/clang/test/Headers/stddefneeds.cpp
+++ b/clang/test/Headers/stddefneeds.cpp
@@ -56,14 +56,21 @@ max_align_t m5;
 #undef NULL
 #define NULL 0
 
-// glibc (and other) headers then define __need_NULL and rely on stddef.h
-// to redefine NULL to the correct value again.
-#define __need_NULL
+// Including stddef.h again shouldn't redefine NULL
 #include <stddef.h>
 
 // gtk headers then use __attribute__((sentinel)), which doesn't work if NULL
 // is 0.
-void f(const char* c, ...) __attribute__((sentinel));
+void f(const char* c, ...) __attribute__((sentinel)); // expected-note{{function has been explicitly marked sentinel here}}
 void g() {
+  f("", NULL); // expected-warning{{missing sentinel in function call}}
+}
+
+// glibc (and other) headers then define __need_NULL and rely on stddef.h
+// to redefine NULL to the correct value again.
+#define __need_NULL
+#include <stddef.h>
+
+void h() {
   f("", NULL);  // Shouldn't warn.
 }

llvmbot · 2024-07-20T00:55:19Z

@llvm/pr-subscribers-clang

Author: Ian Anderson (ian-twilightcoder)

Changes

stddef.h always includes __stddef_null.h. This is fine in modules because it's not possible to re-include the pcm, and it's necessary to export the _Builtin_stddef.null submodule. However, without modules it causes NULL to always get redefined which disrupts some C++ code. Rework the inclusion of __stddef_null.h so that with not building with modules it's only included if __need_NULL is set by the includer, or it's the first time stddef.h is being included.

Full diff: https://github.com/llvm/llvm-project/pull/99727.diff

3 Files Affected:

(modified) clang/lib/Headers/stdarg.h (+2-2)
(modified) clang/lib/Headers/stddef.h (+20-2)
(modified) clang/test/Headers/stddefneeds.cpp (+11-4)

diff --git a/clang/lib/Headers/stdarg.h b/clang/lib/Headers/stdarg.h
index 8292ab907becf..6203d7a600a23 100644
--- a/clang/lib/Headers/stdarg.h
+++ b/clang/lib/Headers/stdarg.h
@@ -20,19 +20,18 @@
  * modules.
  */
 #if defined(__MVS__) && __has_include_next(<stdarg.h>)
-#include <__stdarg_header_macro.h>
 #undef __need___va_list
 #undef __need_va_list
 #undef __need_va_arg
 #undef __need___va_copy
 #undef __need_va_copy
+#include <__stdarg_header_macro.h>
 #include_next <stdarg.h>
 
 #else
 #if !defined(__need___va_list) && !defined(__need_va_list) &&                  \
     !defined(__need_va_arg) && !defined(__need___va_copy) &&                   \
     !defined(__need_va_copy)
-#include <__stdarg_header_macro.h>
 #define __need___va_list
 #define __need_va_list
 #define __need_va_arg
@@ -45,6 +44,7 @@
     !defined(__STRICT_ANSI__)
 #define __need_va_copy
 #endif
+#include <__stdarg_header_macro.h>
 #endif
 
 #ifdef __need___va_list
diff --git a/clang/lib/Headers/stddef.h b/clang/lib/Headers/stddef.h
index 8985c526e8fc5..dab464805ccea 100644
--- a/clang/lib/Headers/stddef.h
+++ b/clang/lib/Headers/stddef.h
@@ -20,7 +20,6 @@
  * modules.
  */
 #if defined(__MVS__) && __has_include_next(<stddef.h>)
-#include <__stddef_header_macro.h>
 #undef __need_ptrdiff_t
 #undef __need_size_t
 #undef __need_rsize_t
@@ -31,6 +30,7 @@
 #undef __need_max_align_t
 #undef __need_offsetof
 #undef __need_wint_t
+#include <__stddef_header_macro.h>
 #include_next <stddef.h>
 
 #else
@@ -40,7 +40,6 @@
     !defined(__need_NULL) && !defined(__need_nullptr_t) &&                     \
     !defined(__need_unreachable) && !defined(__need_max_align_t) &&            \
     !defined(__need_offsetof) && !defined(__need_wint_t)
-#include <__stddef_header_macro.h>
 #define __need_ptrdiff_t
 #define __need_size_t
 /* ISO9899:2011 7.20 (C11 Annex K): Define rsize_t if __STDC_WANT_LIB_EXT1__ is
@@ -49,7 +48,25 @@
 #define __need_rsize_t
 #endif
 #define __need_wchar_t
+#if !defined(__STDDEF_H) && !__building_module(_Builtin_stddef)
+/*
+ * __stddef_null.h is special when building without modules: if __need_NULL is
+ * set, then it will unconditionally redefine NULL. To avoid stepping on client
+ * definitions of NULL, __need_NULL should only be set the first time this
+ * header is included, that is when __STDDEF_H is not defined. However, when
+ * building with modules, this header is a textual header and needs to
+ * unconditionally include __stdef_null.h to support multiple submodules
+ * exporting _Builtin_stddef.null. Take module SM with submodules A and B, whose
+ * headers both include stddef.h When SM.A builds, __STDDEF_H will be defined.
+ * When SM.B builds, the definition from SM.A will leak when building without
+ * local submodule visibility. stddef.h wouldn't include any of its
+ * implementation headers, and SM.B wouldn't import any of the stddef modules,
+ * and SM.B's `export *` wouldn't export any stddef interfaces as expected. When
+ * building with modules, always include __stddef_null.h so that everything
+ * works as expected.
+ */
 #define __need_NULL
+#endif
 #if (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202311L) ||              \
     defined(__cplusplus)
 #define __need_nullptr_t
@@ -65,6 +82,7 @@
 /* wint_t is provided by <wchar.h> and not <stddef.h>. It's here
  * for compatibility, but must be explicitly requested. Therefore
  * __need_wint_t is intentionally not defined here. */
+#include <__stddef_header_macro.h>
 #endif
 
 #if defined(__need_ptrdiff_t)
diff --git a/clang/test/Headers/stddefneeds.cpp b/clang/test/Headers/stddefneeds.cpp
index 0763bbdee13ae..0282e8afa600d 100644
--- a/clang/test/Headers/stddefneeds.cpp
+++ b/clang/test/Headers/stddefneeds.cpp
@@ -56,14 +56,21 @@ max_align_t m5;
 #undef NULL
 #define NULL 0
 
-// glibc (and other) headers then define __need_NULL and rely on stddef.h
-// to redefine NULL to the correct value again.
-#define __need_NULL
+// Including stddef.h again shouldn't redefine NULL
 #include <stddef.h>
 
 // gtk headers then use __attribute__((sentinel)), which doesn't work if NULL
 // is 0.
-void f(const char* c, ...) __attribute__((sentinel));
+void f(const char* c, ...) __attribute__((sentinel)); // expected-note{{function has been explicitly marked sentinel here}}
 void g() {
+  f("", NULL); // expected-warning{{missing sentinel in function call}}
+}
+
+// glibc (and other) headers then define __need_NULL and rely on stddef.h
+// to redefine NULL to the correct value again.
+#define __need_NULL
+#include <stddef.h>
+
+void h() {
   f("", NULL);  // Shouldn't warn.
 }

clang/lib/Headers/stddef.h

AaronBallman

These changes should come with a release note so users know about the fix.

clang/lib/Headers/stdarg.h

ian-twilightcoder · 2024-07-22T18:25:34Z

These changes should come with a release note so users know about the fix.

Does it need a release note? It was introduced in 19 with #90676, and this will fix it in 19 too.

AaronBallman · 2024-07-22T18:32:57Z

These changes should come with a release note so users know about the fix.

Does it need a release note? It was introduced in 19 with #90676, and this will fix it in 19 too.

Ah nope, no need for a release note then!

AaronBallman

With additional test coverage, the changes LGTM but I'd appreciate it if @zygoloid or @ChuanqiXu9 could validate the modules logic.

clang/lib/Headers/stddef.h

stddef.h always includes __stddef_null.h. This is fine in modules because it's not possible to re-include the pcm, and it's necessary to export the _Builtin_stddef.null submodule. However, without modules it causes NULL to always get redefined which disrupts some C++ code. Rework the inclusion of __stddef_null.h so that with not building with modules it's only included if __need_NULL is set by the includer, or it's the first time stddef.h is being included.

ldionne · 2024-07-23T16:51:11Z

@AaronBallman Should this be added to the 19.x milestone and cherry-picked?

AaronBallman · 2024-07-23T17:14:17Z

@AaronBallman Should this be added to the 19.x milestone and cherry-picked?

I think it should be

ian-twilightcoder · 2024-07-23T17:50:39Z

@AaronBallman Should this be added to the 19.x milestone and cherry-picked?

I think it should be

Nuts, should've merged it yesterday.

ldionne · 2024-07-23T17:52:27Z

@ian-twilightcoder Cherry-picking is easy. Once this is merged, follow the guidelines here to get it cherry-picked: https://llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches

Basically you only need to add a comment to this PR like /cherry-pick COMMIT-SHA and everything else pretty much happens automatically.

llvmbot · 2024-07-23T17:57:13Z

Failed to cherry-pick: COMMIT-SHA`

https://github.com/llvm/llvm-project/actions/runs/10063850061

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

ian-twilightcoder · 2024-07-23T18:19:23Z

The Driver/linker-wrapper-passes.c test failure doesn't look related, that test doesn't use stddef.h or anything else. I keep trying to rebase to make it go away but it's stubborn.

AaronBallman

The Driver/linker-wrapper-passes.c test failure doesn't look related, that test doesn't use stddef.h or anything else. I keep trying to rebase to make it go away but it's stubborn.

I looked at the failures as well and I think they're unrelated.

This LGTM and based on the last comment from @zygoloid, I think this is ready to land (and be cherry-picked to the 19.x release).

Thank you for the fix!

ian-twilightcoder · 2024-07-23T20:03:23Z

/cherry-pick 92a9d48

ian-twilightcoder · 2024-07-23T20:05:13Z

@ian-twilightcoder Cherry-picking is easy. Once this is merged, follow the guidelines here to get it cherry-picked: https://llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches

Basically you only need to add a comment to this PR like /cherry-pick COMMIT-SHA and everything else pretty much happens automatically.

Oh I thought you had to make a GitHub issue and get a branch manager to merge it for you. Ok let me give it a try 🤞

stddef.h always includes __stddef_null.h. This is fine in modules because it's not possible to re-include the pcm, and it's necessary to export the _Builtin_stddef.null submodule. However, without modules it causes NULL to always get redefined which disrupts some C++ code. Rework the inclusion of __stddef_null.h so that with not building with modules it's only included if __need_NULL is set by the includer, or it's the first time stddef.h is being included. (cherry picked from commit 92a9d48)

llvmbot · 2024-07-23T20:08:31Z

/pull-request #100191

llvmbot · 2024-07-23T20:09:57Z

Failed to cherry-pick: COMMIT-SHA`

https://github.com/llvm/llvm-project/actions/runs/10065620100

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

ldionne · 2024-07-24T15:06:49Z

@ian-twilightcoder You can create an issue if you want, but you can also say /cherry-pick directly from the PR as long as you add the release milestone to it.

That being said, the cherry-pick failed so you will need to create a PR manually.

llvmbot · 2024-07-24T15:11:38Z

Failed to cherry-pick:

https://github.com/llvm/llvm-project/actions/runs/10079206235

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

Summary: stddef.h always includes __stddef_null.h. This is fine in modules because it's not possible to re-include the pcm, and it's necessary to export the _Builtin_stddef.null submodule. However, without modules it causes NULL to always get redefined which disrupts some C++ code. Rework the inclusion of __stddef_null.h so that with not building with modules it's only included if __need_NULL is set by the includer, or it's the first time stddef.h is being included. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251032

stddef.h always includes __stddef_null.h. This is fine in modules because it's not possible to re-include the pcm, and it's necessary to export the _Builtin_stddef.null submodule. However, without modules it causes NULL to always get redefined which disrupts some C++ code. Rework the inclusion of __stddef_null.h so that with not building with modules it's only included if __need_NULL is set by the includer, or it's the first time stddef.h is being included. (cherry picked from commit 92a9d48)

ian-twilightcoder requested review from Bigcheese, ldionne and AaronBallman July 20, 2024 00:54

llvmbot added clang Clang issues not falling into any other category backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics labels Jul 20, 2024

ian-twilightcoder mentioned this pull request Jul 20, 2024

[libc++] Simplify the implementation of <stddef.h> #86843

Merged

zygoloid reviewed Jul 20, 2024

View reviewed changes

clang/lib/Headers/stddef.h Outdated Show resolved Hide resolved

ian-twilightcoder force-pushed the null_overwrite branch 3 times, most recently from 4b8c087 to e956b0d Compare July 22, 2024 16:58

AaronBallman reviewed Jul 22, 2024

View reviewed changes

clang/lib/Headers/stdarg.h Show resolved Hide resolved

AaronBallman requested a review from ChuanqiXu9 July 22, 2024 18:34

AaronBallman reviewed Jul 22, 2024

View reviewed changes

ian-twilightcoder requested a review from zygoloid July 22, 2024 18:41

ian-twilightcoder force-pushed the null_overwrite branch from e956b0d to 58361b6 Compare July 22, 2024 19:33

llvmbot added the clang:modules C++20 modules and Clang Header Modules label Jul 22, 2024

ian-twilightcoder force-pushed the null_overwrite branch 2 times, most recently from 29915cd to af616b8 Compare July 22, 2024 20:19

zygoloid reviewed Jul 22, 2024

View reviewed changes

clang/lib/Headers/stddef.h Show resolved Hide resolved

ian-twilightcoder force-pushed the null_overwrite branch 2 times, most recently from f494a4c to 5e50bd5 Compare July 22, 2024 23:55

ian-twilightcoder force-pushed the null_overwrite branch from 5e50bd5 to bb9a412 Compare July 23, 2024 16:17

ldionne added this to the LLVM 19.X Release milestone Jul 23, 2024

llvmbot added the release:cherry-pick-failed label Jul 23, 2024

AaronBallman removed the release:cherry-pick-failed label Jul 23, 2024

AaronBallman approved these changes Jul 23, 2024

View reviewed changes

ian-twilightcoder merged commit 92a9d48 into llvm:main Jul 23, 2024
6 of 8 checks passed

llvmbot added the release:cherry-pick-failed label Jul 23, 2024

philnik777 mentioned this pull request Aug 2, 2024

[Clang] Add a release note deprecating __is_nullptr #101638

Merged

mgabka mentioned this pull request Aug 22, 2024

Add release note about ABI mgabka/llvm-project#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang][headers] Including stddef.h always redefines NULL #99727

[clang][headers] Including stddef.h always redefines NULL #99727

ian-twilightcoder commented Jul 20, 2024

llvmbot commented Jul 20, 2024 •

edited

Loading

llvmbot commented Jul 20, 2024

AaronBallman left a comment

ian-twilightcoder commented Jul 22, 2024

AaronBallman commented Jul 22, 2024

AaronBallman left a comment

ldionne commented Jul 23, 2024

AaronBallman commented Jul 23, 2024

ian-twilightcoder commented Jul 23, 2024

ldionne commented Jul 23, 2024

llvmbot commented Jul 23, 2024

ian-twilightcoder commented Jul 23, 2024

AaronBallman left a comment

ian-twilightcoder commented Jul 23, 2024

ian-twilightcoder commented Jul 23, 2024

llvmbot commented Jul 23, 2024

llvmbot commented Jul 23, 2024

ldionne commented Jul 24, 2024

llvmbot commented Jul 24, 2024

[clang][headers] Including stddef.h always redefines NULL #99727

[clang][headers] Including stddef.h always redefines NULL #99727

Conversation

ian-twilightcoder commented Jul 20, 2024

llvmbot commented Jul 20, 2024 • edited Loading

llvmbot commented Jul 20, 2024

AaronBallman left a comment

Choose a reason for hiding this comment

ian-twilightcoder commented Jul 22, 2024

AaronBallman commented Jul 22, 2024

AaronBallman left a comment

Choose a reason for hiding this comment

ldionne commented Jul 23, 2024

AaronBallman commented Jul 23, 2024

ian-twilightcoder commented Jul 23, 2024

ldionne commented Jul 23, 2024

llvmbot commented Jul 23, 2024

ian-twilightcoder commented Jul 23, 2024

AaronBallman left a comment

Choose a reason for hiding this comment

ian-twilightcoder commented Jul 23, 2024

ian-twilightcoder commented Jul 23, 2024

llvmbot commented Jul 23, 2024

llvmbot commented Jul 23, 2024

ldionne commented Jul 24, 2024

llvmbot commented Jul 24, 2024

llvmbot commented Jul 20, 2024 •

edited

Loading