From a649e0a6e80f5ae26657b640de469e8a53244ad2 Mon Sep 17 00:00:00 2001 From: Ian Anderson Date: Wed, 13 Mar 2024 11:15:41 -0700 Subject: [clang][modules] giving the __stddef_ headers their own modules can cause redeclaration errors with -fbuiltin-headers-in-system-modules (#84127) On Apple platforms, some of the stddef.h types are also declared in system headers. In particular NULL has a conflicting declaration in . 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 f50d3582b4844b86ad86372028e44b52c560ec7d) --- clang/lib/Basic/Module.cpp | 7 ++--- clang/lib/Headers/__stddef_null.h | 2 +- clang/lib/Headers/__stddef_nullptr_t.h | 7 ++++- clang/lib/Headers/__stddef_offsetof.h | 7 ++++- clang/lib/Headers/__stddef_ptrdiff_t.h | 7 ++++- clang/lib/Headers/__stddef_rsize_t.h | 7 ++++- clang/lib/Headers/__stddef_size_t.h | 7 ++++- clang/lib/Headers/__stddef_unreachable.h | 7 ++++- clang/lib/Headers/__stddef_wchar_t.h | 7 ++++- clang/lib/Headers/module.modulemap | 20 ++++++-------- clang/lib/Lex/ModuleMap.cpp | 9 ++++-- .../Modules/no-undeclared-includes-builtins.cpp | 2 +- clang/test/Modules/stddef.c | 32 ++++++++++++---------- 13 files changed, 80 insertions(+), 41 deletions(-) diff --git a/clang/lib/Basic/Module.cpp b/clang/lib/Basic/Module.cpp index 9252174..0dac874 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 7336fda..c10bd2d 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 183d394..7f3fbe6 100644 --- a/clang/lib/Headers/__stddef_nullptr_t.h +++ b/clang/lib/Headers/__stddef_nullptr_t.h @@ -7,7 +7,12 @@ *===-----------------------------------------------------------------------=== */ -#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 3b347b3..84172c6 100644 --- a/clang/lib/Headers/__stddef_offsetof.h +++ b/clang/lib/Headers/__stddef_offsetof.h @@ -7,6 +7,11 @@ *===-----------------------------------------------------------------------=== */ -#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 3ea6d7d..fd3c893 100644 --- a/clang/lib/Headers/__stddef_ptrdiff_t.h +++ b/clang/lib/Headers/__stddef_ptrdiff_t.h @@ -7,7 +7,12 @@ *===-----------------------------------------------------------------------=== */ -#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 b6428d0..dd433d4 100644 --- a/clang/lib/Headers/__stddef_rsize_t.h +++ b/clang/lib/Headers/__stddef_rsize_t.h @@ -7,7 +7,12 @@ *===-----------------------------------------------------------------------=== */ -#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 e4a3895..3dd7b1f 100644 --- a/clang/lib/Headers/__stddef_size_t.h +++ b/clang/lib/Headers/__stddef_size_t.h @@ -7,7 +7,12 @@ *===-----------------------------------------------------------------------=== */ -#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 3e7fe01..518580c 100644 --- a/clang/lib/Headers/__stddef_unreachable.h +++ b/clang/lib/Headers/__stddef_unreachable.h @@ -7,6 +7,11 @@ *===-----------------------------------------------------------------------=== */ -#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 16a6186..bd69f63 100644 --- a/clang/lib/Headers/__stddef_wchar_t.h +++ b/clang/lib/Headers/__stddef_wchar_t.h @@ -9,7 +9,12 @@ #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 a786689..56a13f6 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 and not . 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 and not . 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 afb2948..10c475f 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 c9bffc5..f9eefd2 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 diff --git a/clang/test/Modules/stddef.c b/clang/test/Modules/stddef.c index 5bc0d1e..7623982 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 which includes <__stddef_size_t.h> which imports the -// _Builtin_stddef.size_t module. +// Includes 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; -- cgit v1.1