[clang] [clang][modules] giving the __stddef_ headers their own modules can cause redeclaration errors with -fbuiltin-headers-in-system-modules (PR #84127)

Ian Anderson via cfe-commits cfe-commits at lists.llvm.org
Tue Mar 5 23:02:44 PST 2024


https://github.com/ian-twilightcoder created https://github.com/llvm/llvm-project/pull/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.

>From 0cc4b77fce06730f6a6a8b242384036018ebfe79 Mon Sep 17 00:00:00 2001
From: Ian Anderson <iana at apple.com>
Date: Tue, 5 Mar 2024 22:56:36 -0800
Subject: [PATCH] [clang][modules] giving the __stddef_ headers their own
 modules can cause 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.
---
 clang/lib/Basic/Module.cpp                    |  7 ++--
 clang/lib/Headers/__stddef_null.h             |  2 +-
 clang/lib/Headers/__stddef_nullptr_t.h        |  6 +++-
 clang/lib/Headers/__stddef_offsetof.h         |  6 +++-
 clang/lib/Headers/__stddef_ptrdiff_t.h        |  6 +++-
 clang/lib/Headers/__stddef_rsize_t.h          |  6 +++-
 clang/lib/Headers/__stddef_size_t.h           |  6 +++-
 clang/lib/Headers/__stddef_unreachable.h      |  6 +++-
 clang/lib/Headers/__stddef_wchar_t.h          |  6 +++-
 clang/lib/Headers/module.modulemap            | 20 ++++++------
 clang/lib/Lex/ModuleMap.cpp                   |  9 ++++--
 .../no-undeclared-includes-builtins.cpp       |  2 +-
 clang/test/Modules/stddef.c                   | 32 +++++++++++--------
 13 files changed, 73 insertions(+), 41 deletions(-)

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 at size_t.h:* 0+ {{here}}
-// expected-note at __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 at size_t.h:* 0+ {{here}} \
+   no-builtin-headers-in-system-modules-note at size_t.h:* 0+ {{here}}
+// builtin-headers-in-system-modules-note at __stddef_size_t.h:* 0+ {{here}} \
+   no-builtin-headers-in-system-modules-note at __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;



More information about the cfe-commits mailing list