[Modules] Add a flag to control builtin headers being in system modules

[iana]

Summary

Including select builtin headers in system modules is a workaround for module cycles, primarily in Apple's Darwin module that includes all of its C standard library headers. The workaround is problematic because it doesn't include all of the builtin headers (inttypes.h is notably absent), and it also doesn't include C++ headers. The straightforward for for this is to make top level modules for all of the C standard library headers and unwind.h in C++, clang, and the OS.

However, doing so in clang before the OS modules are ready re-introduces the module cycles. Add a -fbuiltin-headers-in-system-modules option to control if the special builtin headers belong to system modules or builtin modules. Pass the option by default for Apple.

[iana]

A big assumption this patch makes is that ModuleMap::isBuiltinHeader is primarily to support Apple's unfortunate module needs. Thus this patch turns that behavior off by default, which makes things work the way one would expect. That is, when usr/include/module.modulemap references stdint.h, that just means usr/include/stdint.h and it doesn't also pull in the clang builtin stdint.h, it doesn't transform usr/include/stdint.h into a textual header, etc. I'm hoping that's acceptable behavior on non-Apple platforms, but if someone knows otherwise please let me know and we can rethink how the option should be defined and set.

Also we need the behavior switch to be a run time thing and not a compile time thing because Apple needs the same version of clang to support both current and old Apple SDKs. Ideally we'll fix newer SDKs to not require the flag, but we'll still need it passed for older SDKs.

[benlangmuir]

but need to be repeatedly included when they're used in system modules

How does this work today? Wouldn't the include guard prevent this?

clang/include/clang/Basic/Features.def

233

This appears to be in a list of // C++ TSes. Near the bottom there is 'Miscellaneous language extensions' which might be a better fit.

clang/lib/Driver/ToolChains/Darwin.cpp

2848

I would have expected the default to be true; do old SDKs not have SDKInfo or something?

2853

Are these really supposed to be 100 or is this a placeholder?

[iana]

clang/include/clang/Driver/Options.td

2955

I don't love this flag name, but I couldn't come up with anything more succinct. It actually does two things.

1. Turns on ModuleMap::isBuiltinHeader behavior, that is the builtin headers get added into the system modules.
2. Causes the module map parser to ignore all of the new builtin modules added in D159064. This is needed before the modules are added to assist with Apple internal staging with the Swift compiler.

2956–2957

I don't fully understand what these two do, but I think they apply here?

clang/lib/Headers/__stddef_ptrdiff_t.h

10 ↗

(On Diff #556191)

This is to support patterns like the Modules/stddef.c test. The module map looks like this.

module StdDef {
  module Other {
    header "other.h" // includes stddef.h
  }
  module PtrDiffT {
    header "ptrdiff_t.h" // defines __needs_ptrdiff_t and includes stddef.h
  }
  module IncludeAgain {
    header "include_again.h" // includes stddef.h
  }
}

With builtin_headers_in_system_modules, __stddef_ptrdiff_t.h will not be in a module. It will first be seen in other.h and define its header guard. When it's seen again in ptrdiff_t.h, it needs to declare ptrdiff_t again, so it needs to ignore its header guard.

Without builtin_headers_in_system_modules, __stddef_ptrdiff_t.h will be in the _Builtin_stddef module. It needs its header guard there so that it's only included once when building the module. Even though ptrdiff_t.h will see the header guard, it will still import _Builtin_stddef.ptrdiff_t and get the declaration.

clang/lib/Headers/stddef.h

10 ↗

(On Diff #556191)

As a textual header, this needs to ignore its header guard in module mode, even without builtin_headers_in_system_modules. Going back to the Modules/stddef.c test.

module StdDef {
  module Other {
    header "other.h" // includes stddef.h
    export *
  }
  module PtrDiffT {
    header "ptrdiff_t.h" // defines __needs_ptrdiff_t and includes stddef.h
  }
  module IncludeAgain {
    header "include_again.h" // includes stddef.h
    export *
  }
}

When other.h compiles, STDDEF_H will get defined and _Builtin_stddef.ptrdiff_t will be imported. But when include_again.h compiles, STDDEF_H will already be defined and _Builtin_stddef.ptrdiff_t won't be imported because the include will be skipped in stddef.h. That's ok when include_again.h is building because it will see ptrdiff_t from when other.h included it. But a client that only includes include_again.h won't see any types because StdDef.IncludeAgain skipped the contents of stddef.h and skipped importing _Builtin_stddef.ptrdiff_t, and so there's nothing for export * to export.

The other solution is to never define __STDDEF_H in module mode, but I think that's strange behavior. Several headers in the Apple SDKs test if a header was included by checking for its header guard (questionable, but it's what they do). stddef.h isn't one of those headers, but it still seems like it should define everything in modules mode that it defines without modules.

clang/lib/Lex/ModuleMap.cpp

259

This had to be moved up just because it's used by methods under here.

1540

This an everything under it is gross, but the only other thing I could think of was to duplicate the module map and decided to load a special one for BuiltinHeadersInSystemModules. That seemed weirder.

clang/test/Modules/Werror-Wsystem-headers.m

6–8

Most of these tests aren't really affected by -fbuiltin-headers-in-system-modules, they just coincidentally use Inputs/System/usr/include and that needs -fbuiltin-headers-in-system-modules.

clang/test/Modules/shadowed-submodule.m

2

This could probably be redone to not use stdarg.h as the shadow and then it wouldn't require -fbuiltin-headers-in-system-modules, but it's probably ok as-is?

clang/test/Modules/stddef.c

2

This one and the next one will be updated in the next review to test without -fbuiltin-headers-in-system-modules too.

[iana]

In D159483#4641231, @benlangmuir wrote:

but need to be repeatedly included when they're used in system modules

How does this work today? Wouldn't the include guard prevent this?

Today they don't define their include guard when building with modules.

[iana]

clang/include/clang/Basic/Features.def

233

I thought it went with FEATURE(modules, LangOpts.Modules), but I can put it down in 'Miscellaneous language extensions' if that's better.

clang/lib/Driver/ToolChains/Darwin.cpp

2848

I'm not sure if we ever expect there to not be SDKInfo, but I thought it safer to default to the current behavior which is false.

2853

Placeholder until we actually update the SDKs.

[benlangmuir]

How does this work today? Wouldn't the include guard prevent this?

Today they don't define their include guard when building with modules.

Thanks - I can see some headers behave that way, or in other cases there are other sorts of has_feature(modules) checks that I can see you're modifying, but what about all the *va_* headers that currently just seem to have normal header guards?

[iana]

In D159483#4641289, @benlangmuir wrote:

How does this work today? Wouldn't the include guard prevent this?

Today they don't define their include guard when building with modules.

Thanks - I can see some headers behave that way, or in other cases there are other sorts of has_feature(modules) checks that I can see you're modifying, but what about all the *va_* headers that currently just seem to have normal header guards?

I think the stdarg ones didn't always work properly in modules and we just got lucky.

[iana]

Fix formatting

[iana]

Rebase

[benlangmuir]

I suggest we add a comment explaining the weird has_feature checks being added here (even if they're less weird than what they're replacing). Maybe just a comment in stdarg.h and/or stddef.h and then the __std(arg|def) headers could just add a one-liner reference "see comment in std(arg|def).h".

clang/include/clang/Basic/Features.def

233

I think misc is probably better.

clang/include/clang/Driver/Options.td

2955

I think the name is good enough for a -cc1 option, unless someone else has a better suggestion. I suggest adding a HelpText - you could basically copy the description you added to LangOptions.def

2956–2957

I agree they apply. f_Group affects a ClaimAllArgs call (via inheritance from CompileOnly_Group) but mostly puts it under a group in documentation. NoXarchOption means you can't modify this option per-arch in a multi-arch compilation. That seems like what you want.

clang/lib/Lex/ModuleMap.cpp

1540

Would it be possible to get the right semantics using a requires decl in the modules plus a new module feature?

[iana]

clang/include/clang/Basic/Features.def

233

👍

clang/include/clang/Driver/Options.td

2955

Sure

clang/lib/Lex/ModuleMap.cpp

1540

Maybe? I guess e.g. _Builtin_stdint would be seen, the new requires flag would be seen, and if it's not satisfied then its headers wouldn't be added to the module? That seems like a really weird use of requires though.

[iana]

Add comments to stdarg.h and stddef.h explaining their interesting header guard setup.
Ignore -fbuiltin-headers-in-system-modules if -fmodules isn't passed.
DriverKit doesn't need to pass -fbuiltin-headers-in-system-modules since it doesn't have a module map at all.
Add a test for -fbuiltin-headers-in-system-modules/__has_feature(builtin_headers_in_system_modules)
Miscellaneous review feedback

[iana]

Rebase

[iana]

__stddef_max_align_t.h always needs a header guard, because the type merger can't handle struct's. If it has a header guard then it always needs to have a module too, even if -fbuiltin-headers-in-system-modules is passed.

Change ModuleMap to not ignore the builtin modules completely, but rather to just skip their top level headers (which is isBuiltinHeaderName + inttypes.h and stdnoreturn.h). With that change, the implementation headers are always modular, and __has_feature(builtin_headers_in_system_modules) is no longer necessary anywhere.

[vsapsai]

In D159483#4641191, @iana wrote:

A big assumption this patch makes is that ModuleMap::isBuiltinHeader is primarily to support Apple's unfortunate module needs. Thus this patch turns that behavior off by default, which makes things work the way one would expect. That is, when usr/include/module.modulemap references stdint.h, that just means usr/include/stdint.h and it doesn't also pull in the clang builtin stdint.h, it doesn't transform usr/include/stdint.h into a textual header, etc. I'm hoping that's acceptable behavior on non-Apple platforms, but if someone knows otherwise please let me know and we can rethink how the option should be defined and set.

Good way to test the assumptions can be checking how Swift works with this change on Linux. There are not as many module maps there as in Apple SDKs but there are some.

clang/include/clang/Basic/LangOptions.def

176

Why not to make the default value true to preserve the old behavior and switch on the new behavior in the driver? It's not a veiled way to force you to change it, I'm genuinely curious and want to consider pro/cons of various alternatives.

clang/lib/Driver/ToolChains/Darwin.cpp

2860–2861

Another question regarding defaults. Doesn't look consistent with your position

[...] I thought it safer to default to the current behavior which is false.

Personally I gravitate towards false for extra safety but it's not a strong opinion.

[iana]

clang/include/clang/Basic/LangOptions.def

176

I did it this way because the current behavior is nonintuitive and just kind of bizarre. I had no idea that Darwin.C.stdint also included the compiler's stdint.h, and that the OS's stdint.h was treated textually. I thought it would be better for the default behavior to be for modules to behave consistently and obviously, not have secret special cases, and be usable with C++.

The con of course is that if anyone besides Apple depends on the current behavior they're going to either have to fix their OS modules or add this flag. I'm not sure if other platforms have a convenient driver class where they can do that.

clang/lib/Driver/ToolChains/Darwin.cpp

2860–2861

default only covers DriverKit, which can be true. Any future platforms should also default to true.

[vsapsai]

clang/include/clang/Basic/LangOptions.def

176

I'm fine with the more aggressive approach as long as we test Swift on Linux. That's the only project I know that is willing to keep using clang modules. For other projects if things break and it is a motivation to move to C++ modules, it is a positive direction, as for me.

Another reason I wanted to be more cautious is because of https://discourse.llvm.org/t/configure-script-breakage-with-the-new-werror-implicit-function-declaration/65213 change. Clang started enforcing 20+ years old rule and that has broken some stuff. So I don't see "you should've known better before relying on this behavior" as a good argument.

Anyway, for Swift testing here is what you can do (if you have your own plan, go ahead):

1. Make PR for the same change as this one at https://github.com/apple/llvm-project/
2. Create inconsequential PR at https://github.com/apple/swift/
3. Use Cross Repository Testing to test clang change from swift on Linux.

clang/lib/Driver/ToolChains/Darwin.cpp

2860–2861

Ok, that looks like a reasonable choice.

[iana]

clang/include/clang/Basic/LangOptions.def

176

If things break I think we would just add the flag to a few more drivers and fix it like that. I'm definitely not trying to drive anyone away from clang modules, I'm just hoping they don't need the strange current behavior. If it turns out to be too much of a mess then it can be on by default and we'll do an fno flag instead that we can turn off in the Darwin driver when we're ready.

[Bigcheese]

The implementation of this change looks good. I share the concern that changing the default may break other users, but that's easily remedied and this is the correct default. It would be good to hear from at least the Google folks before landing though.

[dblaikie]

@sammccall @rsmith - figure if this does impact us, we'll use the flag to opt-out in the short term while we figure out longer-term solution, yeah? Is there any pre-emptive testing you think is worth doing?

[mib]

@iana I believe this is affecting lldb macOS bot: https://green.lab.llvm.org/green/view/LLDB/job/as-lldb-cmake/6316/

Can you take a look ? Thanks!

clang/include/clang/Basic/Features.def

 FEATURE(is_sealed, LangOpts.CPlusPlus &&LangOpts.MicrosoftExt)
 FEATURE(is_trivial, LangOpts.CPlusPlus)
 FEATURE(is_trivially_assignable, LangOpts.CPlusPlus)
 FEATURE(is_trivially_constructible, LangOpts.CPlusPlus)
 FEATURE(is_trivially_copyable, LangOpts.CPlusPlus)
 FEATURE(is_union, LangOpts.CPlusPlus)
 FEATURE(kcfi, LangOpts.Sanitize.has(SanitizerKind::KCFI))
 FEATURE(modules, LangOpts.Modules)
 FEATURE(safe_stack, LangOpts.Sanitize.has(SanitizerKind::SafeStack))
 FEATURE(shadow_call_stack,
         LangOpts.Sanitize.has(SanitizerKind::ShadowCallStack))
 FEATURE(tls, PP.getTargetInfo().isTLSSupported())
 FEATURE(underlying_type, LangOpts.CPlusPlus)
 FEATURE(experimental_library, LangOpts.ExperimentalLibrary)
 
 // C11 features supported by other languages as extensions.
 EXTENSION(c_alignas, true)

 EXTENSION(statement_attributes_with_gnu_syntax, true)
 EXTENSION(gnu_asm, LangOpts.GNUAsm)
 EXTENSION(gnu_asm_goto_with_outputs, LangOpts.GNUAsm)
 EXTENSION(gnu_asm_goto_with_outputs_full, LangOpts.GNUAsm)
 EXTENSION(matrix_types, LangOpts.MatrixTypes)
 EXTENSION(matrix_types_scalar_division, true)
 EXTENSION(cxx_attributes_on_using_declarations, LangOpts.CPlusPlus11)
 
+FEATURE(builtin_headers_in_system_modules, LangOpts.BuiltinHeadersInSystemModules)
 FEATURE(cxx_abi_relative_vtable, LangOpts.CPlusPlus && LangOpts.RelativeCXXABIVTables)
 
 // CUDA/HIP Features
 FEATURE(cuda_noinline_keyword, LangOpts.CUDA)
 
 #undef EXTENSION
 #undef FEATURE

clang/include/clang/Basic/LangOptions.def

 BENIGN_LANGOPT(ThreadsafeStatics , 1, 1, "thread-safe static initializers")
 LANGOPT(POSIXThreads      , 1, 0, "POSIX thread support")
 LANGOPT(Blocks            , 1, 0, "blocks extension to C")
 BENIGN_LANGOPT(EmitAllDecls      , 1, 0, "emitting all declarations")
 LANGOPT(MathErrno         , 1, 1, "errno in math functions")
 BENIGN_LANGOPT(HeinousExtensions , 1, 0, "extensions that we really don't like and may be ripped out at any time")
 LANGOPT(Modules           , 1, 0, "modules semantics")
 COMPATIBLE_LANGOPT(CPlusPlusModules, 1, 0, "C++ modules syntax")
+LANGOPT(BuiltinHeadersInSystemModules, 1, 0, "builtin headers belong to system modules, and _Builtin_ modules are ignored for cstdlib headers")
 BENIGN_ENUM_LANGOPT(CompilingModule, CompilingModuleKind, 3, CMK_None,
                     "compiling a module interface")
 BENIGN_LANGOPT(CompilingPCH, 1, 0, "building a pch")
 BENIGN_LANGOPT(BuildingPCHWithObjectFile, 1, 0, "building a pch which has a corresponding object file")
 BENIGN_LANGOPT(CacheGeneratedPCH, 1, 0, "cache generated PCH files in memory")
 BENIGN_LANGOPT(PCHInstantiateTemplates, 1, 0, "instantiate templates while building a PCH")
 COMPATIBLE_LANGOPT(ModulesDeclUse    , 1, 0, "require declaration of module uses")
 BENIGN_LANGOPT(ModulesSearchAll  , 1, 1, "searching even non-imported modules to find unresolved references")

clang/include/clang/Driver/Options.td

def fimplicit_module_maps : Flag <["-"], "fimplicit-module-maps">, Group<f_Group>,
   HelpText<"Implicitly search the file system for module map files.">,
   MarshallingInfoFlag<HeaderSearchOpts<"ImplicitModuleMaps">>;
 defm modules : BoolFOption<"modules",
   LangOpts<"Modules">, Default<fcxx_modules.KeyPath>,
   PosFlag<SetTrue, [], [ClangOption, CC1Option],
           "Enable the 'modules' language feature">,
   NegFlag<SetFalse>, BothFlags<
           [NoXarchOption], [ClangOption, CLOption]>>;
+def fbuiltin_headers_in_system_modules : Flag <["-"], "fbuiltin-headers-in-system-modules">,
+  Group<f_Group>,
+  Flags<[NoXarchOption]>,
+  Visibility<[CC1Option]>,
+  ShouldParseIf<fmodules.KeyPath>,
+  HelpText<"builtin headers belong to system modules, and _Builtin_ modules are ignored for cstdlib headers">,
+  MarshallingInfoFlag<LangOpts<"BuiltinHeadersInSystemModules">>;
 def fmodule_maps : Flag <["-"], "fmodule-maps">,
   Visibility<[ClangOption, CLOption]>, Alias<fimplicit_module_maps>;
 def fmodule_name_EQ : Joined<["-"], "fmodule-name=">, Group<f_Group>,
   Flags<[NoXarchOption]>,
   Visibility<[ClangOption, CC1Option, CLOption]>,
   MetaVarName<"<name>">,
   HelpText<"Specify the name of the module to build">,
   MarshallingInfoString<LangOpts<"ModuleName">>;

clang/include/clang/Lex/ModuleMap.h

public:
   }
 
   /// Get the directory that contains Clang-supplied include files.
   const DirectoryEntry *getBuiltinDir() const {
     return BuiltinIncludeDir;
   }
 
   /// Is this a compiler builtin header?
-  static bool isBuiltinHeader(StringRef FileName);
   bool isBuiltinHeader(const FileEntry *File);
 
   /// Add a module map callback.
   void addModuleMapCallbacks(std::unique_ptr<ModuleMapCallbacks> Callback) {
     Callbacks.push_back(std::move(Callback));
   }
 
   /// Retrieve the module that owns the given header file, if any. Note that

clang/lib/Driver/ToolChains/Darwin.cpp

case WatchOS: // Earlier than 4.0.
     break;
   case DriverKit: // Always available.
     return false;
   }
 
   return TargetVersion < alignedAllocMinVersion(OS);
 }
 
+static bool sdkSupportsBuiltinModules(const Darwin::DarwinPlatformKind &TargetPlatform, const std::optional<DarwinSDKInfo> &SDKInfo) {
+  if (!SDKInfo)
+    return false;
+
+  VersionTuple SDKVersion = SDKInfo->getVersion();
+  switch (TargetPlatform) {
+  case Darwin::MacOS:
+    return SDKVersion >= VersionTuple(99U);
+  case Darwin::IPhoneOS:
+    return SDKVersion >= VersionTuple(99U);
+  case Darwin::TvOS:
+    return SDKVersion >= VersionTuple(99U);
+  case Darwin::WatchOS:
+    return SDKVersion >= VersionTuple(99U);
+  default:
+    return true;
+  }
+}
+
 void Darwin::addClangTargetOptions(const llvm::opt::ArgList &DriverArgs,
                                    llvm::opt::ArgStringList &CC1Args,
                                    Action::OffloadKind DeviceOffloadKind) const {
   // Pass "-faligned-alloc-unavailable" only when the user hasn't manually
   // enabled or disabled aligned allocations.
   if (!DriverArgs.hasArgNoClaim(options::OPT_faligned_allocation,
                                 options::OPT_fno_aligned_allocation) &&
       isAlignedAllocationUnavailable())
     CC1Args.push_back("-faligned-alloc-unavailable");
 
   addClangCC1ASTargetOptions(DriverArgs, CC1Args);
 
   // Enable compatibility mode for NSItemProviderCompletionHandler in
   // Foundation/NSItemProvider.h.
   CC1Args.push_back("-fcompatibility-qualified-id-block-type-checking");
 
   // Give static local variables in inline functions hidden visibility when
   // -fvisibility-inlines-hidden is enabled.
   if (!DriverArgs.getLastArgNoClaim(
           options::OPT_fvisibility_inlines_hidden_static_local_var,
           options::OPT_fno_visibility_inlines_hidden_static_local_var))
     CC1Args.push_back("-fvisibility-inlines-hidden-static-local-var");
+
+  // Earlier versions of the darwin SDK have the C standard library headers
+  // all together in the Darwin module. That leads to module cycles with
+  // the _Builtin_ modules. e.g. <inttypes.h> on darwin includes <stdint.h>.
+  // The builtin <stdint.h> include-nexts <stdint.h>. When both of those
+  // darwin headers are in the Darwin module, there's a module cycle Darwin ->
+  // _Builtin_stdint -> Darwin (i.e. inttypes.h (darwin) -> stdint.h (builtin) ->
+  // stdint.h (darwin)). This is fixed in later versions of the darwin SDK,
+  // but until then, the builtin headers need to join the system modules.
+  // i.e. when the builtin stdint.h is in the Darwin module too, the cycle
+  // goes away. Note that -fbuiltin-headers-in-system-modules does nothing
+  // to fix the same problem with C++ headers, and is generally fragile.
+  if (!sdkSupportsBuiltinModules(TargetPlatform, SDKInfo))
+    CC1Args.push_back("-fbuiltin-headers-in-system-modules");
 }
 
 void Darwin::addClangCC1ASTargetOptions(
     const llvm::opt::ArgList &Args, llvm::opt::ArgStringList &CC1ASArgs) const {
   if (TargetVariantTriple) {
     CC1ASArgs.push_back("-darwin-target-variant-triple");
     CC1ASArgs.push_back(Args.MakeArgString(TargetVariantTriple->getTriple()));
   }

clang/lib/Lex/ModuleMap.cpp

if (GetFrameworkFile()) {
       NeedsFramework = true;
     }
     return std::nullopt;
   }
 
   return NormalHdrFile;
 }
 
+/// Determine whether the given file name is the name of a builtin
+/// header, supplied by Clang to replace, override, or augment existing system
+/// headers.
+static bool isBuiltinHeaderName(StringRef FileName) {
+  return llvm::StringSwitch<bool>(FileName)
+           .Case("float.h", true)
+           .Case("iso646.h", true)
+           .Case("limits.h", true)
+           .Case("stdalign.h", true)
+           .Case("stdarg.h", true)
+           .Case("stdatomic.h", true)
+           .Case("stdbool.h", true)
+           .Case("stddef.h", true)
+           .Case("stdint.h", true)
+           .Case("tgmath.h", true)
+           .Case("unwind.h", true)
+           .Default(false);
+}
+
+/// Determine whether the given module name is the name of a builtin
+/// module that is cyclic with a system module  on some platforms.
+static bool isBuiltInModuleName(StringRef ModuleName) {
+  return llvm::StringSwitch<bool>(ModuleName)
+           .Case("_Builtin_float", true)
+           .Case("_Builtin_inttypes", true)
+           .Case("_Builtin_iso646", true)
+           .Case("_Builtin_limits", true)
+           .Case("_Builtin_stdalign", true)
+           .Case("_Builtin_stdarg", true)
+           .Case("_Builtin_stdatomic", true)
+           .Case("_Builtin_stdbool", true)
+           .Case("_Builtin_stddef", true)
+           .Case("_Builtin_stdint", true)
+           .Case("_Builtin_stdnoreturn", true)
+           .Case("_Builtin_tgmath", true)
+           .Case("_Builtin_unwind", true)
+           .Default(false);
+}
+
 void ModuleMap::resolveHeader(Module *Mod,
                               const Module::UnresolvedHeaderDirective &Header,
                               bool &NeedsFramework) {
   SmallString<128> RelativePathName;
   if (OptionalFileEntryRef File =
           findHeader(Mod, Header, RelativePathName, NeedsFramework)) {
     if (Header.IsUmbrella) {
       const DirectoryEntry *UmbrellaDir = &File->getDir().getDirEntry();

 }
 
 bool ModuleMap::resolveAsBuiltinHeader(
     Module *Mod, const Module::UnresolvedHeaderDirective &Header) {
   if (Header.Kind == Module::HK_Excluded ||
       llvm::sys::path::is_absolute(Header.FileName) ||
       Mod->isPartOfFramework() || !Mod->IsSystem || Header.IsUmbrella ||
       !BuiltinIncludeDir || BuiltinIncludeDir == Mod->Directory ||
-      !isBuiltinHeader(Header.FileName))
+      !LangOpts.BuiltinHeadersInSystemModules || !isBuiltinHeaderName(Header.FileName))
     return false;
 
   // This is a system module with a top-level header. This header
   // may have a counterpart (or replacement) in the set of headers
   // supplied by Clang. Find that builtin header.
   SmallString<128> Path;
   llvm::sys::path::append(Path, BuiltinIncludeDir->getName(), Header.FileName);
   auto File = SourceMgr.getFileManager().getOptionalFileRef(Path);
if (Name.data() != Buffer.data())
       Buffer.append(Name.begin(), Name.end());
     Buffer.push_back('_');
     Name = StringRef(Buffer.data(), Buffer.size());
   }
 
   return Name;
 }
 
-/// Determine whether the given file name is the name of a builtin
-/// header, supplied by Clang to replace, override, or augment existing system
-/// headers.
-bool ModuleMap::isBuiltinHeader(StringRef FileName) {
-  return llvm::StringSwitch<bool>(FileName)
-           .Case("float.h", true)
-           .Case("iso646.h", true)
-           .Case("limits.h", true)
-           .Case("stdalign.h", true)
-           .Case("stdarg.h", true)
-           .Case("stdatomic.h", true)
-           .Case("stdbool.h", true)
-           .Case("stddef.h", true)
-           .Case("stdint.h", true)
-           .Case("tgmath.h", true)
-           .Case("unwind.h", true)
-           .Default(false);
-}
-
 bool ModuleMap::isBuiltinHeader(const FileEntry *File) {
-  return File->getDir() == BuiltinIncludeDir &&
-         ModuleMap::isBuiltinHeader(llvm::sys::path::filename(File->getName()));
+  return File->getDir() == BuiltinIncludeDir && LangOpts.BuiltinHeadersInSystemModules &&
+         isBuiltinHeaderName(llvm::sys::path::filename(File->getName()));
 }
 
 ModuleMap::HeadersMap::iterator
 ModuleMap::findKnownHeader(const FileEntry *File) {
   resolveHeaderDirectives(File);
   HeadersMap::iterator Known = Headers.find(File);
   if (HeaderInfo.getHeaderSearchOpts().ImplicitModuleMaps &&
       Known == Headers.end() && ModuleMap::isBuiltinHeader(File)) {
class ModuleMapParser {
     /// The current token.
     MMToken Tok;
 
     /// The active module.
     Module *ActiveModule = nullptr;
 
     /// Whether a module uses the 'requires excluded' hack to mark its
     /// contents as 'textual'.
     ///
     /// On older Darwin SDK versions, 'requires excluded' is used to mark the
     /// contents of the Darwin.C.excluded (assert.h) and Tcl.Private modules as
     /// non-modular headers.  For backwards compatibility, we continue to
     /// support this idiom for just these modules, and map the headers to
     /// 'textual' to match the original intent.
     llvm::SmallPtrSet<Module *, 2> UsesRequiresExcludedHack;
 
     /// Consume the current token and return its location.
if (Tok.is(MMToken::LBrace)) {
     else {
       Diags.Report(Tok.getLocation(), diag::err_mmap_expected_rbrace);
       Diags.Report(LBraceLoc, diag::note_mmap_lbrace_match);
       HadError = true;
     }
   }
 
   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))
     Map.addUnresolvedHeader(ActiveModule, std::move(Header), NeedsFramework);
 
   if (NeedsFramework)
     Diags.Report(CurrModuleDeclLoc, diag::note_mmap_add_framework_keyword)
       << ActiveModule->getFullModuleName()
       << FixItHint::CreateReplacement(CurrModuleDeclLoc, "framework module");
 }
 
 static int compareModuleHeaders(const Module::Header *A,

clang/test/Driver/Inputs/MacOSX99.0.sdk/SDKSettings.json

+{"Version":"990.0", "MaximumDeploymentTarget": "99.0.99"}

clang/test/Driver/darwin-builtin-modules.c

+// Check that darwin passes -fbuiltin-headers-in-system-modules
+// when expected.
+
+// RUN: %clang -target x86_64-apple-darwin22.4 -### %s 2>&1 | FileCheck %s
+// RUN: %clang -isysroot %S/Inputs/MacOSX10.15.versioned.sdk -target x86_64-apple-macos10.15 -### %s 2>&1 | FileCheck %s
+// RUN: %clang -isysroot %S/Inputs/iPhoneOS13.0.sdk -target arm64-apple-ios13.0 -### %s 2>&1 | FileCheck %s
+// CHECK: -fbuiltin-headers-in-system-modules
+
+// RUN: %clang -isysroot %S/Inputs/MacOSX99.0.sdk -target x86_64-apple-macos98.0 -### %s 2>&1 | FileCheck --check-prefix=CHECK_FUTURE %s
+// RUN: %clang -isysroot %S/Inputs/MacOSX99.0.sdk -target x86_64-apple-macos99.0 -### %s 2>&1 | FileCheck --check-prefix=CHECK_FUTURE %s
+// CHECK_FUTURE-NOT: -fbuiltin-headers-in-system-modules
+
+
+// Check that builtin_headers_in_system_modules is only set if -fbuiltin-headers-in-system-modules and -fmodules are both set.
+
+// RUN: %clang -isysroot %S/Inputs/iPhoneOS13.0.sdk -target arm64-apple-ios13.0 -fsyntax-only %s -Xclang -verify=no-feature
+// RUN: %clang -isysroot %S/Inputs/iPhoneOS13.0.sdk -target arm64-apple-ios13.0 -fsyntax-only %s -fmodules -Xclang -verify=yes-feature
+// RUN: %clang -isysroot %S/Inputs/MacOSX99.0.sdk -target x86_64-apple-macos99.0 -fsyntax-only %s -Xclang -verify=no-feature
+// RUN: %clang -isysroot %S/Inputs/MacOSX99.0.sdk -target x86_64-apple-macos99.0 -fsyntax-only %s -fmodules -Xclang -verify=no-feature
+
+#if __has_feature(builtin_headers_in_system_modules)
+#error "has builtin_headers_in_system_modules"
+// yes-feature-error@-1 {{}}
+#else
+#error "no builtin_headers_in_system_modules"
+// no-feature-error@-1 {{}}
+#endif

clang/test/Modules/Werror-Wsystem-headers.m

 // RUN: rm -rf %t
 // RUN: rm -rf %t-saved
 // RUN: mkdir %t-saved
 
 // Initial module build
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash \
-// RUN:     -isystem %S/Inputs/System/usr/include -fsyntax-only %s -verify
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fbuiltin-headers-in-system-modules \
+// RUN:     -fmodules-cache-path=%t -fdisable-module-hash -isystem %S/Inputs/System/usr/include \
+// RUN:     -fsyntax-only %s -verify
 // RUN: cp %t/cstd.pcm %t-saved/cstd.pcm
 
 // Even with -Werror don't rebuild a system module
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash \
-// RUN:     -isystem %S/Inputs/System/usr/include -fsyntax-only %s -verify -Werror
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fbuiltin-headers-in-system-modules \
+// RUN:     -fmodules-cache-path=%t -fdisable-module-hash -isystem %S/Inputs/System/usr/include \
+// RUN:     -fsyntax-only %s -verify -Werror
 // RUN: diff %t/cstd.pcm %t-saved/cstd.pcm
 
 // Unless -Wsystem-headers is on
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash \
-// RUN:     -isystem %S/Inputs/System/usr/include -fsyntax-only %s -verify \
-// RUN:     -Werror=unused -Wsystem-headers
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fbuiltin-headers-in-system-modules \
+// RUN:     -fmodules-cache-path=%t -fdisable-module-hash -isystem %S/Inputs/System/usr/include \
+// RUN:     -fsyntax-only %s -verify -Werror=unused -Wsystem-headers
 // RUN: not diff %t/cstd.pcm %t-saved/cstd.pcm
 
 // expected-no-diagnostics
 @import cstd;

clang/test/Modules/context-hash.c

 // This test verifies that only strict hashing includes search paths and
 // diagnostics in the module context hash.
 
 // RUN: rm -rf %t
 // RUN: %clang_cc1 -fsyntax-only -internal-isystem \
 // RUN:   %S/Inputs/System/usr/include -fmodules -fimplicit-module-maps \
-// RUN:   -fmodules-cache-path=%t %s -Rmodule-build 2> %t1
+// RUN:   -fbuiltin-headers-in-system-modules -fmodules-cache-path=%t %s \
+// RUN:   -Rmodule-build 2> %t1
 // RUN: rm -rf %t
 // RUN: %clang_cc1 -fsyntax-only -internal-isystem \
 // RUN:   %S/Inputs/System/usr/include -internal-isystem %S -fmodules \
-// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t %s -Rmodule-build 2> \
-// RUN:   %t2
+// RUN:   -fbuiltin-headers-in-system-modules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t %s -Rmodule-build 2> %t2
 // RUN: rm -rf %t
 // RUN: %clang_cc1 -fsyntax-only -internal-isystem \
 // RUN:   %S/Inputs/System/usr/include -internal-isystem %S -fmodules \
-// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t %s \
-// RUN:   -fmodules-strict-context-hash -Rmodule-build 2> %t3
+// RUN:   -fbuiltin-headers-in-system-modules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t %s -fmodules-strict-context-hash \
+// RUN:   -Rmodule-build 2> %t3
 // RUN: rm -rf %t
 // RUN: %clang_cc1 -fsyntax-only -Weverything -internal-isystem \
 // RUN:   %S/Inputs/System/usr/include -fmodules -fmodules-strict-context-hash \
-// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t %s -Rmodule-build 2> \
-// RUN:   %t4
+// RUN:   -fbuiltin-headers-in-system-modules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t %s -Rmodule-build 2> %t4
 // RUN: echo %t > %t.path
 // RUN: cat %t.path %t1 %t2 %t3 %t4 | FileCheck %s
 
 // This tests things verified by ASTReader::checkLanguageOptions that are not
 // part of LangOpts.def.
 
 // RUN: rm -rf %t
 // RUN: %clang_cc1 -fsyntax-only -internal-isystem \
 // RUN:   %S/Inputs/System/usr/include -fmodules -fimplicit-module-maps \
-// RUN:   -fmodules-cache-path=%t -x objective-c %s -Rmodule-build 2> %t1
+// RUN:   -fbuiltin-headers-in-system-modules -fmodules-cache-path=%t \
+// RUN:   -x objective-c %s -Rmodule-build 2> %t1
 // RUN: rm -rf %t
 // RUN: %clang_cc1 -fsyntax-only -internal-isystem \
 // RUN:   %S/Inputs/System/usr/include -fmodules -fimplicit-module-maps \
-// RUN:   -fobjc-runtime=macosx-1.0.0.0 \
+// RUN:   -fbuiltin-headers-in-system-modules -fobjc-runtime=macosx-1.0.0.0 \
 // RUN:   -fmodules-cache-path=%t -x objective-c %s -Rmodule-build 2> %t2
 // RUN: rm -rf %t
 // RUN: %clang_cc1 -fsyntax-only -internal-isystem \
 // RUN:   %S/Inputs/System/usr/include -fmodules -fimplicit-module-maps \
-// RUN:   -fcomment-block-commands=lp,bj \
+// RUN:   -fbuiltin-headers-in-system-modules -fcomment-block-commands=lp,bj \
 // RUN:   -fmodules-cache-path=%t -x objective-c %s -Rmodule-build 2> %t3
 // RUN: echo %t > %t.path
 // RUN: cat %t.path %t1 %t2 %t3 | FileCheck --check-prefix=LANGOPTS %s
 
 #include <stdio.h>
 
 // CHECK: [[PREFIX:(.*[/\\])+[a-zA-Z0-9.-]+]]
 // CHECK: building module 'cstd' as '[[PREFIX]]{{[/\\]}}[[CONTEXT_HASH:[A-Z0-9]+]]{{[/\\]}}cstd-[[AST_HASH:[A-Z0-9]+]].pcm'

clang/test/Modules/crash-vfs-include-pch.m

 // REQUIRES: crash-recovery, shell, system-darwin
 //
 // RUN: rm -rf %t
 // RUN: mkdir -p %t/m %t/out
 
 // RUN: %clang_cc1 -x objective-c-header -emit-pch %S/Inputs/pch-used.h \
 // RUN:     -o %t/out/pch-used.h.pch -fmodules -fimplicit-module-maps \
-// RUN:     -fmodules-cache-path=%t/cache -O0 \
+// RUN:     -fbuiltin-headers-in-system-modules -fmodules-cache-path=%t/cache -O0 \
 // RUN:     -isystem %S/Inputs/System/usr/include
 
 // RUN: env FORCE_CLANG_DIAGNOSTICS_CRASH= TMPDIR=%t TEMP=%t TMP=%t \
 // RUN: not %clang %s -E -include-pch %t/out/pch-used.h.pch -fmodules -nostdlibinc \
-// RUN:     -fimplicit-module-maps -fmodules-cache-path=%t/cache -O0 \
-// RUN:     -Xclang -fno-validate-pch -isystem %S/Inputs/System/usr/include \
-// RUN:     -o %t/output.E 2>&1 | FileCheck %s
+// RUN:     -fimplicit-module-maps -fbuiltin-headers-in-system-modules \
+// RUN:     -fmodules-cache-path=%t/cache -O0 -Xclang -fno-validate-pch \
+// RUN:     -isystem %S/Inputs/System/usr/include -o %t/output.E 2>&1 | FileCheck %s
 
 // RUN: FileCheck --check-prefix=CHECKSH %s -input-file %t/crash-vfs-*.sh
 // RUN: FileCheck --check-prefix=CHECKYAML %s -input-file \
 // RUN:   %t/crash-vfs-*.cache/vfs/vfs.yaml
 
 void f() { SPXTrace(); }
 void g() { double x = DBL_MAX; }
 

clang/test/Modules/cstd.m

 // RUN: rm -rf %t
-// RUN: %clang_cc1 -fsyntax-only -internal-isystem %S/Inputs/System/usr/include -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -D__need_wint_t -Werror=implicit-function-declaration %s
+// RUN: %clang_cc1 -fsyntax-only -internal-isystem %S/Inputs/System/usr/include -fmodules -fimplicit-module-maps -fbuiltin-headers-in-system-modules -fmodules-cache-path=%t -D__need_wint_t -Werror=implicit-function-declaration %s
 
 @import uses_other_constants;
 const double other_value = DBL_MAX;
 
 // Supplied by compiler, but referenced from the "/usr/include" module map.
 @import cstd.float_constants;
 
 float getFltMax(void) { return FLT_MAX; }

clang/test/Modules/pch-used.m

 // UNSUPPORTED: target={{.*}}-zos{{.*}}, target={{.*}}-aix{{.*}}
 // RUN: rm -rf %t
 // RUN: mkdir %t
-// RUN: %clang_cc1 -x objective-c-header -emit-pch %S/Inputs/pch-used.h -o %t/pch-used.h.pch -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -O0 -isystem %S/Inputs/System/usr/include
-// RUN: %clang_cc1 %s -include-pch %t/pch-used.h.pch -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -O0 -isystem %S/Inputs/System/usr/include -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -x objective-c-header -emit-pch %S/Inputs/pch-used.h -o %t/pch-used.h.pch -fmodules -fimplicit-module-maps -fbuiltin-headers-in-system-modules -fmodules-cache-path=%t/cache -O0 -isystem %S/Inputs/System/usr/include
+// RUN: %clang_cc1 %s -include-pch %t/pch-used.h.pch -fmodules -fimplicit-module-maps -fbuiltin-headers-in-system-modules -fmodules-cache-path=%t/cache -O0 -isystem %S/Inputs/System/usr/include -emit-llvm -o - | FileCheck %s
 
 void f(void) { SPXTrace(); }
 void g(void) { double x = DBL_MAX; }
 
 // CHECK: define internal {{.*}}void @SPXTrace

clang/test/Modules/shadowed-submodule.m

 // RUN: rm -rf %t
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/shadowed-submodule/Foo -I %S/Inputs/shadowed-submodule/A2 %s -verify
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fbuiltin-headers-in-system-modules -fmodules-cache-path=%t -I %S/Inputs/shadowed-submodule/Foo -I %S/Inputs/shadowed-submodule/A2 %s -verify
 
 @import Foo; // expected-error {{module 'A' was built in directory}}
              // expected-note@shadowed-submodule.m:4 {{imported by module 'Foo'}}

clang/test/Modules/stddef.c

 // 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 -fbuiltin-headers-in-system-modules -fmodules-cache-path=%t -I%S/Inputs/StdDef %s -verify -fno-modules-error-recovery
 
 #include "ptrdiff_t.h"
 
 ptrdiff_t pdt;
 
 size_t st; // expected-error {{missing '#include "include_again.h"'; 'size_t' must be declared before it is used}}
 // expected-note@__stddef_size_t.h:* {{here}}
 
 #include "include_again.h"
 
 size_t st2;

clang/test/Modules/stddef.m

 @import StdDef.Other;
 
 size_t getSize(void);
 
 // RUN: rm -rf %t
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/StdDef %s -verify
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fbuiltin-headers-in-system-modules -fmodules-cache-path=%t -I %S/Inputs/StdDef %s -verify
 // expected-no-diagnostics