[clang][modules] giving the __stddef_ headers their own modules can cause redeclaration errors with -fbuiltin-headers-in-system-modules #84127
+80
−41
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ian-twilightcoder
requested review from
zygoloid,
Bigcheese,
vsapsai,
benlangmuir,
AaronBallman,
ributzka,
efriedma-quic and
ChuanqiXu9
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-clang Author: Ian Anderson (ian-twilightcoder) ChangesOn Apple platforms, some of the stddef.h types are also declared in system headers. In particular NULL has a conflicting declaration in <sys/_types/_null.h>. When that's in a different module from <__stddef_null.h>, redeclaration errors can occur. Make the _stddef headers be non-modular in -fbuiltin-headers-in-system-modules and restore them back to not respecting their header guards. Still define the header guards though. __stddef_max_align_t.h was in _Builtin_stddef_max_align_t prior to the addition of _Builtin_stddef, and it needs to stay in a module because struct's can't be type merged. __stddef_wint_t.h didn't used to have a module, but leave it in it current module since it doesn't really belong to stddef.h. Full diff: https://github.com/llvm/llvm-project/pull/84127.diff 13 Files Affected:
diff --git a/clang/lib/Basic/Module.cpp b/clang/lib/Basic/Module.cpp
index 1c5043a618fff3..f68a556fb455bf 100644
--- a/clang/lib/Basic/Module.cpp
+++ b/clang/lib/Basic/Module.cpp
@@ -301,10 +301,9 @@ bool Module::directlyUses(const Module *Requested) {
if (Requested->isSubModuleOf(Use))
return true;
- // Anyone is allowed to use our builtin stdarg.h and stddef.h and their
- // accompanying modules.
- if (Requested->getTopLevelModuleName() == "_Builtin_stdarg" ||
- Requested->getTopLevelModuleName() == "_Builtin_stddef")
+ // Anyone is allowed to use our builtin stddef.h and its accompanying modules.
+ if (Requested->fullModuleNameIs({"_Builtin_stddef", "max_align_t"}) ||
+ Requested->fullModuleNameIs({"_Builtin_stddef_wint_t"}))
return true;
if (NoUndeclaredIncludes)
diff --git a/clang/lib/Headers/__stddef_null.h b/clang/lib/Headers/__stddef_null.h
index 7336fdab389723..c10bd2d7d9887c 100644
--- a/clang/lib/Headers/__stddef_null.h
+++ b/clang/lib/Headers/__stddef_null.h
@@ -7,7 +7,7 @@
*===-----------------------------------------------------------------------===
*/
-#if !defined(NULL) || !__has_feature(modules)
+#if !defined(NULL) || !__building_module(_Builtin_stddef)
/* linux/stddef.h will define NULL to 0. glibc (and other) headers then define
* __need_NULL and rely on stddef.h to redefine NULL to the correct value again.
diff --git a/clang/lib/Headers/__stddef_nullptr_t.h b/clang/lib/Headers/__stddef_nullptr_t.h
index 183d394d56c1b7..d724b5cba18294 100644
--- a/clang/lib/Headers/__stddef_nullptr_t.h
+++ b/clang/lib/Headers/__stddef_nullptr_t.h
@@ -7,7 +7,11 @@
*===-----------------------------------------------------------------------===
*/
-#ifndef _NULLPTR_T
+/*
+ * When -fbuiltin-headers-in-system-modules is set this is a non-modular header
+ * and needs to behave as if it was textual.
+ */
+#if !defined(_NULLPTR_T) || (__has_feature(modules) && !__building_module(_Builtin_stddef))
#define _NULLPTR_T
#ifdef __cplusplus
diff --git a/clang/lib/Headers/__stddef_offsetof.h b/clang/lib/Headers/__stddef_offsetof.h
index 3b347b3b92f62c..62c49c78bd0516 100644
--- a/clang/lib/Headers/__stddef_offsetof.h
+++ b/clang/lib/Headers/__stddef_offsetof.h
@@ -7,6 +7,10 @@
*===-----------------------------------------------------------------------===
*/
-#ifndef offsetof
+/*
+ * When -fbuiltin-headers-in-system-modules is set this is a non-modular header
+ * and needs to behave as if it was textual.
+ */
+#if !defined(offsetof) || (__has_feature(modules) && !__building_module(_Builtin_stddef))
#define offsetof(t, d) __builtin_offsetof(t, d)
#endif
diff --git a/clang/lib/Headers/__stddef_ptrdiff_t.h b/clang/lib/Headers/__stddef_ptrdiff_t.h
index 3ea6d7d2852e1c..31ecc00b624602 100644
--- a/clang/lib/Headers/__stddef_ptrdiff_t.h
+++ b/clang/lib/Headers/__stddef_ptrdiff_t.h
@@ -7,7 +7,11 @@
*===-----------------------------------------------------------------------===
*/
-#ifndef _PTRDIFF_T
+/*
+ * When -fbuiltin-headers-in-system-modules is set this is a non-modular header
+ * and needs to behave as if it was textual.
+ */
+#if !defined(_PTRDIFF_T) || (__has_feature(modules) && !__building_module(_Builtin_stddef))
#define _PTRDIFF_T
typedef __PTRDIFF_TYPE__ ptrdiff_t;
diff --git a/clang/lib/Headers/__stddef_rsize_t.h b/clang/lib/Headers/__stddef_rsize_t.h
index b6428d0c12b62a..9cb9c266116cf7 100644
--- a/clang/lib/Headers/__stddef_rsize_t.h
+++ b/clang/lib/Headers/__stddef_rsize_t.h
@@ -7,7 +7,11 @@
*===-----------------------------------------------------------------------===
*/
-#ifndef _RSIZE_T
+/*
+ * When -fbuiltin-headers-in-system-modules is set this is a non-modular header
+ * and needs to behave as if it was textual.
+ */
+#if !defined(_RSIZE_T) || (__has_feature(modules) && !__building_module(_Builtin_stddef))
#define _RSIZE_T
typedef __SIZE_TYPE__ rsize_t;
diff --git a/clang/lib/Headers/__stddef_size_t.h b/clang/lib/Headers/__stddef_size_t.h
index e4a389510bcdbf..01d3a0c4802c70 100644
--- a/clang/lib/Headers/__stddef_size_t.h
+++ b/clang/lib/Headers/__stddef_size_t.h
@@ -7,7 +7,11 @@
*===-----------------------------------------------------------------------===
*/
-#ifndef _SIZE_T
+/*
+ * When -fbuiltin-headers-in-system-modules is set this is a non-modular header
+ * and needs to behave as if it was textual.
+ */
+#if !defined(_SIZE_T) || (__has_feature(modules) && !__building_module(_Builtin_stddef))
#define _SIZE_T
typedef __SIZE_TYPE__ size_t;
diff --git a/clang/lib/Headers/__stddef_unreachable.h b/clang/lib/Headers/__stddef_unreachable.h
index 3e7fe01979662a..1f9254f6bb5753 100644
--- a/clang/lib/Headers/__stddef_unreachable.h
+++ b/clang/lib/Headers/__stddef_unreachable.h
@@ -7,6 +7,10 @@
*===-----------------------------------------------------------------------===
*/
-#ifndef unreachable
+/*
+ * When -fbuiltin-headers-in-system-modules is set this is a non-modular header
+ * and needs to behave as if it was textual.
+ */
+#if !defined(unreachable) || (__has_feature(modules) && !__building_module(_Builtin_stddef))
#define unreachable() __builtin_unreachable()
#endif
diff --git a/clang/lib/Headers/__stddef_wchar_t.h b/clang/lib/Headers/__stddef_wchar_t.h
index 16a6186512c0c3..4239f619f085d9 100644
--- a/clang/lib/Headers/__stddef_wchar_t.h
+++ b/clang/lib/Headers/__stddef_wchar_t.h
@@ -9,7 +9,11 @@
#if !defined(__cplusplus) || (defined(_MSC_VER) && !_NATIVE_WCHAR_T_DEFINED)
-#ifndef _WCHAR_T
+/*
+ * When -fbuiltin-headers-in-system-modules is set this is a non-modular header
+ * and needs to behave as if it was textual.
+ */
+#if !defined(_WCHAR_T) || (__has_feature(modules) && !__building_module(_Builtin_stddef))
#define _WCHAR_T
#ifdef _MSC_EXTENSIONS
diff --git a/clang/lib/Headers/module.modulemap b/clang/lib/Headers/module.modulemap
index a786689d391773..56a13f69bc0559 100644
--- a/clang/lib/Headers/module.modulemap
+++ b/clang/lib/Headers/module.modulemap
@@ -155,9 +155,9 @@ module _Builtin_intrinsics [system] [extern_c] {
// Start -fbuiltin-headers-in-system-modules affected modules
-// The following modules all ignore their top level headers
-// when -fbuiltin-headers-in-system-modules is passed, and
-// most of those headers join system modules when present.
+// The following modules all ignore their headers when
+// -fbuiltin-headers-in-system-modules is passed, and many of
+// those headers join system modules when present.
// e.g. if -fbuiltin-headers-in-system-modules is passed, then
// float.h will not be in the _Builtin_float module (that module
@@ -190,11 +190,6 @@ module _Builtin_stdalign [system] {
export *
}
-// When -fbuiltin-headers-in-system-modules is passed, only
-// the top level headers are removed, the implementation headers
-// will always be in their submodules. That means when stdarg.h
-// is included, it will still import this module and make the
-// appropriate submodules visible.
module _Builtin_stdarg [system] {
textual header "stdarg.h"
@@ -237,6 +232,8 @@ module _Builtin_stdbool [system] {
module _Builtin_stddef [system] {
textual header "stddef.h"
+ // __stddef_max_align_t.h is always in this module, even if
+ // -fbuiltin-headers-in-system-modules is passed.
explicit module max_align_t {
header "__stddef_max_align_t.h"
export *
@@ -283,9 +280,10 @@ module _Builtin_stddef [system] {
}
}
-/* wint_t is provided by <wchar.h> and not <stddef.h>. It's here
- * for compatibility, but must be explicitly requested. Therefore
- * __stddef_wint_t.h is not part of _Builtin_stddef. */
+// wint_t is provided by <wchar.h> and not <stddef.h>. It's here
+// for compatibility, but must be explicitly requested. Therefore
+// __stddef_wint_t.h is not part of _Builtin_stddef. It is always in
+// this module even if -fbuiltin-headers-in-system-modules is passed.
module _Builtin_stddef_wint_t [system] {
header "__stddef_wint_t.h"
export *
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index afb2948f05ae5b..10c475f617d485 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -2498,9 +2498,12 @@ void ModuleMapParser::parseHeaderDecl(MMToken::TokenKind LeadingToken,
}
bool NeedsFramework = false;
- // Don't add the top level headers to the builtin modules if the builtin headers
- // belong to the system modules.
- if (!Map.LangOpts.BuiltinHeadersInSystemModules || ActiveModule->isSubModule() || !isBuiltInModuleName(ActiveModule->Name))
+ // Don't add headers to the builtin modules if the builtin headers belong to
+ // the system modules, with the exception of __stddef_max_align_t.h which
+ // always had its own module.
+ if (!Map.LangOpts.BuiltinHeadersInSystemModules ||
+ !isBuiltInModuleName(ActiveModule->getTopLevelModuleName()) ||
+ ActiveModule->fullModuleNameIs({"_Builtin_stddef", "max_align_t"}))
Map.addUnresolvedHeader(ActiveModule, std::move(Header), NeedsFramework);
if (NeedsFramework)
diff --git a/clang/test/Modules/no-undeclared-includes-builtins.cpp b/clang/test/Modules/no-undeclared-includes-builtins.cpp
index c9bffc55619905..f9eefd24a33c7d 100644
--- a/clang/test/Modules/no-undeclared-includes-builtins.cpp
+++ b/clang/test/Modules/no-undeclared-includes-builtins.cpp
@@ -8,7 +8,7 @@
// headers.
// RUN: rm -rf %t
-// RUN: %clang_cc1 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs/no-undeclared-includes-builtins/libcxx -I %S/Inputs/no-undeclared-includes-builtins/glibc %s
+// RUN: %clang_cc1 -fmodules-cache-path=%t -fmodules -fbuiltin-headers-in-system-modules -fimplicit-module-maps -I %S/Inputs/no-undeclared-includes-builtins/libcxx -I %S/Inputs/no-undeclared-includes-builtins/glibc %s
// expected-no-diagnostics
#include <stddef.h>
diff --git a/clang/test/Modules/stddef.c b/clang/test/Modules/stddef.c
index 5bc0d1e44c8563..76239826146810 100644
--- a/clang/test/Modules/stddef.c
+++ b/clang/test/Modules/stddef.c
@@ -1,29 +1,33 @@
// RUN: rm -rf %t
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fbuiltin-headers-in-system-modules -fmodules-cache-path=%t -I%S/Inputs/StdDef %s -verify -fno-modules-error-recovery
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fbuiltin-headers-in-system-modules -fmodules-cache-path=%t -I%S/Inputs/StdDef %s -verify=builtin-headers-in-system-modules -fno-modules-error-recovery
// RUN: rm -rf %t
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I%S/Inputs/StdDef %s -verify -fno-modules-error-recovery
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I%S/Inputs/StdDef %s -verify=no-builtin-headers-in-system-modules -fno-modules-error-recovery
#include "ptrdiff_t.h"
ptrdiff_t pdt;
-// size_t is declared in both size_t.h and __stddef_size_t.h, both of which are
-// modular headers. Regardless of whether stddef.h joins the StdDef test module
-// or is in its _Builtin_stddef module, __stddef_size_t.h will be in
-// _Builtin_stddef.size_t. It's not defined which module will win as the expected
-// provider of size_t. For the purposes of this test it doesn't matter which header
-// gets reported, just as long as it isn't other.h or include_again.h.
-size_t st; // expected-error-re {{missing '#include "{{size_t|__stddef_size_t}}.h"'; 'size_t' must be declared before it is used}}
-// expected-note@size_t.h:* 0+ {{here}}
-// expected-note@__stddef_size_t.h:* 0+ {{here}}
+// size_t is declared in both size_t.h and __stddef_size_t.h. If
+// -fbuiltin-headers-in-system-modules is set, then __stddef_size_t.h is a
+// non-modular header that will be transitively pulled in the StdDef test module
+// by include_again.h. Otherwise it will be in the _Builtin_stddef module. In
+// any case it's not defined which module will win as the expected provider of
+// size_t. For the purposes of this test it doesn't matter which of the two
+// providing headers get reported.
+size_t st; // builtin-headers-in-system-modules-error-re {{missing '#include "{{size_t|include_again}}.h"'; 'size_t' must be declared before it is used}} \
+ no-builtin-headers-in-system-modules-error-re {{missing '#include "{{size_t|__stddef_size_t}}.h"'; 'size_t' must be declared before it is used}}
+// builtin-headers-in-system-modules-note@size_t.h:* 0+ {{here}} \
+ no-builtin-headers-in-system-modules-note@size_t.h:* 0+ {{here}}
+// builtin-headers-in-system-modules-note@__stddef_size_t.h:* 0+ {{here}} \
+ no-builtin-headers-in-system-modules-note@__stddef_size_t.h:* 0+ {{here}}
#include "include_again.h"
-// Includes <stddef.h> which includes <__stddef_size_t.h> which imports the
-// _Builtin_stddef.size_t module.
+// Includes <stddef.h> which includes <__stddef_size_t.h>.
size_t st2;
#include "size_t.h"
-// Redeclares size_t, but the type merger should figure it out.
+// Redeclares size_t when -fbuiltin-headers-in-system-modules is not passed, but
+// the type merger should figure it out.
size_t st3;
|
|
We should consider this for LLVM 18, it's a regression from https://reviews.llvm.org/D159064 |
…ause redeclaration errors with -fbuiltin-headers-in-system-modules On Apple platforms, some of the stddef.h types are also declared in system headers. In particular NULL has a conflicting declaration in <sys/_types/_null.h>. When that's in a different module from <__stddef_null.h>, redeclaration errors can occur. Make the __stddef_ headers be non-modular in -fbuiltin-headers-in-system-modules and restore them back to not respecting their header guards. Still define the header guards though. __stddef_max_align_t.h was in _Builtin_stddef_max_align_t prior to the addition of _Builtin_stddef, and it needs to stay in a module because struct's can't be type merged. __stddef_wint_t.h didn't used to have a module, but leave it in it current module since it doesn't really belong to stddef.h.
ian-twilightcoder
force-pushed
the
stddef-submodules
branch
from
0cc4b77
to
e34ccad
Compare
|
Still need to check the code but
sounds quite questionable. |
They were non-modular prior to https://reviews.llvm.org/D159064 |
vsapsai
reviewed
Mar 12, 2024
|
I'm not excited by the complexity we are moving toward with the builtin headers. But I don't have any alternatives. Given the situation we are in, I think the change is ok. But I'd like someone else to look at it as it is tricky to reason about it. No blockers from me but want more people to review the change. |
It's not my favorite. Hopefully Apple will be able to move off of |
benlangmuir
reviewed
Mar 13, 2024
Even for the case with |
benlangmuir
approved these changes
Mar 13, 2024
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Mar 13, 2024
…ause redeclaration errors with -fbuiltin-headers-in-system-modules (llvm#84127) On Apple platforms, some of the stddef.h types are also declared in system headers. In particular NULL has a conflicting declaration in <sys/_types/_null.h>. When that's in a different module from <__stddef_null.h>, redeclaration errors can occur. Make the \_\_stddef_ headers be non-modular in -fbuiltin-headers-in-system-modules and restore them back to not respecting their header guards. Still define the header guards though. __stddef_max_align_t.h was in _Builtin_stddef_max_align_t prior to the addition of _Builtin_stddef, and it needs to stay in a module because struct's can't be type merged. __stddef_wint_t.h didn't used to have a module, but leave it in it current module since it doesn't really belong to stddef.h. (cherry picked from commit f50d358)
ian-twilightcoder
pushed a commit
to ian-twilightcoder/llvm-project
that referenced
this pull request
Mar 14, 2024
…aders their own modules can cause redeclaration errors with -fbuiltin-headers-in-system-modules (llvm#84127) On Apple platforms, some of the stddef.h types are also declared in system headers. In particular NULL has a conflicting declaration in <sys/_types/_null.h>. When that's in a different module from <__stddef_null.h>, redeclaration errors can occur. Make the \_\_stddef_ headers be non-modular in -fbuiltin-headers-in-system-modules and restore them back to not respecting their header guards. Still define the header guards though. __stddef_max_align_t.h was in _Builtin_stddef_max_align_t prior to the addition of _Builtin_stddef, and it needs to stay in a module because struct's can't be type merged. __stddef_wint_t.h didn't used to have a module, but leave it in it current module since it doesn't really belong to stddef.h. rdar://121557777
ian-twilightcoder
added a commit
to ian-twilightcoder/llvm-project
that referenced
this pull request
Mar 14, 2024
apple@ab562ed (llvm#84127) reverses the part of D159064 that originally broke these tests. This reverts commit 2bbc0e4. rdar://124628894
ian-twilightcoder
added a commit
to apple/llvm-project
that referenced
this pull request
Mar 15, 2024
[cherry-pick stable/20230725][clang][modules] giving the __stddef_ headers their own modules can cause redeclaration errors with -fbuiltin-headers-in-system-modules (llvm#84127)
ian-twilightcoder
added a commit
to ian-twilightcoder/llvm-project
that referenced
this pull request
Mar 15, 2024
…ncy scanning test after D159064" apple@ab562ed (llvm#84127) reverses the part of D159064 that originally broke these tests. This reverts commit 33825da. rdar://124628894
tstellar
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Mar 16, 2024
…ause redeclaration errors with -fbuiltin-headers-in-system-modules (llvm#84127) On Apple platforms, some of the stddef.h types are also declared in system headers. In particular NULL has a conflicting declaration in <sys/_types/_null.h>. When that's in a different module from <__stddef_null.h>, redeclaration errors can occur. Make the \_\_stddef_ headers be non-modular in -fbuiltin-headers-in-system-modules and restore them back to not respecting their header guards. Still define the header guards though. __stddef_max_align_t.h was in _Builtin_stddef_max_align_t prior to the addition of _Builtin_stddef, and it needs to stay in a module because struct's can't be type merged. __stddef_wint_t.h didn't used to have a module, but leave it in it current module since it doesn't really belong to stddef.h. (cherry picked from commit f50d358)
ian-twilightcoder
added a commit
to ian-twilightcoder/llvm-project
that referenced
this pull request
Mar 17, 2024
…ncy scanning test after D159064" apple@ab562ed (llvm#84127) reverses the part of D159064 that originally broke these tests. This reverts commit 33825da. rdar://124628894
rastogishubham
pushed a commit
to rastogishubham/llvm-project
that referenced
this pull request
Mar 21, 2024
…ncy scanning test after D159064" apple@ab562ed (llvm#84127) reverses the part of D159064 that originally broke these tests. This reverts commit 33825da. rdar://124628894 (cherry picked from commit a92be41)
Closed
qiaojbao
pushed a commit
to GPUOpen-Drivers/llvm-project
that referenced
this pull request
May 15, 2024
…020611ed6 Local branch amd-gfx 75f0206 Merged main:8a8ef1cacfcd7745d2b6ad00431e6fa9ab9a2fb4 into amd-gfx:c6ac0bd36762 Remote branch main f50d358 [clang][modules] giving the __stddef_ headers their own modules can cause redeclaration errors with -fbuiltin-headers-in-system-modules (llvm#84127)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On Apple platforms, some of the stddef.h types are also declared in system headers. In particular NULL has a conflicting declaration in <sys/_types/_null.h>. When that's in a different module from <__stddef_null.h>, redeclaration errors can occur.
Make the __stddef_ headers be non-modular in -fbuiltin-headers-in-system-modules and restore them back to not respecting their header guards. Still define the header guards though. __stddef_max_align_t.h was in _Builtin_stddef_max_align_t prior to the addition of _Builtin_stddef, and it needs to stay in a module because struct's can't be type merged. __stddef_wint_t.h didn't used to have a module, but leave it in it current module since it doesn't really belong to stddef.h.