[clang] [clang][modules] Non-modular #import Should Respect Submodule Visibility (PR #170215)

[Qiongsi Wu via cfe-commits]

https://github.com/qiongsiwu updated https://github.com/llvm/llvm-project/pull/170215

>From 5ad5394b88b9e07d23905fc0835fb3a99139609c Mon Sep 17 00:00:00 2001
From: Jan Svoboda <jan_svoboda at apple.com>
Date: Thu, 2 Nov 2023 14:35:30 -0700
Subject: [PATCH 1/8] [clang][modules] Track included files per submodule

(cherry picked from commit 9debc58d5135fbde51967dfb076d0ec5d954df38)

 Conflicts:
	clang/include/clang/Lex/Preprocessor.h
	clang/lib/Serialization/ASTReader.cpp
	clang/lib/Serialization/ASTReaderInternals.h
	clang/lib/Serialization/ASTWriter.cpp
---
 clang/include/clang/Basic/Module.h          |  2 +
 clang/include/clang/Lex/Preprocessor.h      | 38 ++++++++++++---
 clang/lib/Serialization/ASTReader.cpp       | 33 ++++++++-----
 clang/lib/Serialization/ASTWriter.cpp       | 46 ++++++++++++++----
 clang/test/Modules/per-submodule-includes.m | 53 +++++++++++++++++++++
 5 files changed, 145 insertions(+), 27 deletions(-)
 create mode 100644 clang/test/Modules/per-submodule-includes.m

diff --git a/clang/include/clang/Basic/Module.h b/clang/include/clang/Basic/Module.h
index 1d9953af057ad..5cf39a8c5e2e1 100644
--- a/clang/include/clang/Basic/Module.h
+++ b/clang/include/clang/Basic/Module.h
@@ -595,6 +595,8 @@ class alignas(8) Module {
   /// to import but didn't because they are not direct uses.
   llvm::SmallSetVector<const Module *, 2> UndeclaredUses;
 
+  llvm::DenseSet<const FileEntry *> Includes;
+
   /// A library or framework to link against when an entity from this
   /// module is used.
   struct LinkLibrary {
diff --git a/clang/include/clang/Lex/Preprocessor.h b/clang/include/clang/Lex/Preprocessor.h
index 8830294ea1658..6cde2eb1fb703 100644
--- a/clang/include/clang/Lex/Preprocessor.h
+++ b/clang/include/clang/Lex/Preprocessor.h
@@ -1052,6 +1052,8 @@ class Preprocessor {
     /// The set of modules that are visible within the submodule.
     VisibleModuleSet VisibleModules;
 
+    /// The files that have been included.
+    IncludedFilesSet IncludedFiles;
     // FIXME: CounterValue?
     // FIXME: PragmaPushMacroInfo?
   };
@@ -1064,8 +1066,8 @@ class Preprocessor {
   /// in a submodule.
   SubmoduleState *CurSubmoduleState;
 
-  /// The files that have been included.
-  IncludedFilesSet IncludedFiles;
+  /// The files that have been included outside of (sub)modules.
+  IncludedFilesSet Includes;
 
   /// The set of top-level modules that affected preprocessing, but were not
   /// imported.
@@ -1550,19 +1552,41 @@ class Preprocessor {
   /// Mark the file as included.
   /// Returns true if this is the first time the file was included.
   bool markIncluded(FileEntryRef File) {
+    bool AlreadyIncluded = alreadyIncluded(File);
     HeaderInfo.getFileInfo(File).IsLocallyIncluded = true;
-    return IncludedFiles.insert(File).second;
+    CurSubmoduleState->IncludedFiles.insert(File);
+    if (!BuildingSubmoduleStack.empty())
+      BuildingSubmoduleStack.back().M->Includes.insert(File);
+    else if (Module *M = getCurrentModule())
+      M->Includes.insert(File);
+    else
+      Includes.insert(File);
+    return !AlreadyIncluded;
   }
 
   /// Return true if this header has already been included.
   bool alreadyIncluded(FileEntryRef File) const {
     HeaderInfo.getFileInfo(File);
-    return IncludedFiles.count(File);
+    if (CurSubmoduleState->IncludedFiles.contains(File))
+      return true;
+    // TODO: Do this more efficiently.
+    for (const auto &[Name, M] : HeaderInfo.getModuleMap().modules())
+      if (CurSubmoduleState->VisibleModules.isVisible(M))
+        if (M->Includes.contains(File))
+          return true;
+    return false;
+  }
+
+  void markIncludedOnTopLevel(const FileEntry *File) {
+    Includes.insert(File);
+    CurSubmoduleState->IncludedFiles.insert(File);
+  }
+
+  void markIncludedInModule(Module *M, const FileEntry *File) {
+    M->Includes.insert(File);
   }
 
-  /// Get the set of included files.
-  IncludedFilesSet &getIncludedFiles() { return IncludedFiles; }
-  const IncludedFilesSet &getIncludedFiles() const { return IncludedFiles; }
+  const IncludedFilesSet &getTopLevelIncludes() const { return Includes; }
 
   /// Return the name of the macro defined before \p Loc that has
   /// spelling \p Tokens.  If there are multiple macros with same spelling,
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 272f756cffb92..bad8a2331efb5 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -2385,14 +2385,6 @@ HeaderFileInfoTrait::ReadData(internal_key_ref key, const unsigned char *d,
   HeaderFileInfo HFI;
   unsigned Flags = *d++;
 
-  OptionalFileEntryRef FE;
-  bool Included = (Flags >> 6) & 0x01;
-  if (Included)
-    if ((FE = getFile(key)))
-      // Not using \c Preprocessor::markIncluded(), since that would attempt to
-      // deserialize this header file info again.
-      Reader.getPreprocessor().getIncludedFiles().insert(*FE);
-
   // FIXME: Refactor with mergeHeaderFileInfo in HeaderSearch.cpp.
   HFI.isImport |= (Flags >> 5) & 0x01;
   HFI.isPragmaOnce |= (Flags >> 4) & 0x01;
@@ -2400,6 +2392,27 @@ HeaderFileInfoTrait::ReadData(internal_key_ref key, const unsigned char *d,
   HFI.LazyControllingMacro = Reader.getGlobalIdentifierID(
       M, endian::readNext<IdentifierID, llvm::endianness::little>(d));
 
+  auto FE = getFile(key);
+  Preprocessor &PP = Reader.getPreprocessor();
+  ModuleMap &ModMap = PP.getHeaderSearchInfo().getModuleMap();
+
+  unsigned IncludedCount =
+      endian::readNext<uint32_t, llvm::endianness::little, unaligned>(d);
+  for (unsigned I = 0; I < IncludedCount; ++I) {
+    uint32_t LocalSMID =
+        endian::readNext<uint32_t, llvm::endianness::little, unaligned>(d);
+    if (!FE)
+      continue;
+
+    if (LocalSMID == 0) {
+      PP.markIncludedOnTopLevel(*FE);
+    } else {
+      SubmoduleID GlobalSMID = Reader.getGlobalSubmoduleID(M, LocalSMID);
+      Module *Mod = Reader.getSubmodule(GlobalSMID);
+      PP.markIncludedInModule(Mod, *FE);
+    }
+  }
+
   assert((End - d) % 4 == 0 &&
          "Wrong data length in HeaderFileInfo deserialization");
   while (d != End) {
@@ -2412,10 +2425,8 @@ HeaderFileInfoTrait::ReadData(internal_key_ref key, const unsigned char *d,
     // implicit module import.
     SubmoduleID GlobalSMID = Reader.getGlobalSubmoduleID(M, LocalSMID);
     Module *Mod = Reader.getSubmodule(GlobalSMID);
-    ModuleMap &ModMap =
-        Reader.getPreprocessor().getHeaderSearchInfo().getModuleMap();
 
-    if (FE || (FE = getFile(key))) {
+    if (FE) {
       // FIXME: NameAsWritten
       Module::Header H = {std::string(key.Filename), "", *FE};
       ModMap.addHeader(Mod, H, HeaderRole, /*Imported=*/true);
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 4d8f922da9044..d16a97610e241 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -2098,14 +2098,15 @@ namespace {
         llvm::PointerIntPair<Module *, 2, ModuleMap::ModuleHeaderRole>;
 
     struct data_type {
-      data_type(const HeaderFileInfo &HFI, bool AlreadyIncluded,
+      data_type(const HeaderFileInfo &HFI,
+                const std::vector<const Module *> &Includers,
                 ArrayRef<ModuleMap::KnownHeader> KnownHeaders,
                 UnresolvedModule Unresolved)
-          : HFI(HFI), AlreadyIncluded(AlreadyIncluded),
-            KnownHeaders(KnownHeaders), Unresolved(Unresolved) {}
+          : HFI(HFI), Includers(Includers), KnownHeaders(KnownHeaders),
+            Unresolved(Unresolved) {}
 
       HeaderFileInfo HFI;
-      bool AlreadyIncluded;
+      std::vector<const Module *> Includers;
       SmallVector<ModuleMap::KnownHeader, 1> KnownHeaders;
       UnresolvedModule Unresolved;
     };
@@ -2128,6 +2129,12 @@ namespace {
     EmitKeyDataLength(raw_ostream& Out, key_type_ref key, data_type_ref Data) {
       unsigned KeyLen = key.Filename.size() + 1 + 8 + 8;
       unsigned DataLen = 1 + sizeof(IdentifierID);
+
+      DataLen += 4;
+      for (const Module *M : Data.Includers)
+        if (!M || Writer.getLocalOrImportedSubmoduleID(M))
+          DataLen += 4;
+
       for (auto ModInfo : Data.KnownHeaders)
         if (Writer.getLocalOrImportedSubmoduleID(ModInfo.getModule()))
           DataLen += 4;
@@ -2154,8 +2161,7 @@ namespace {
       endian::Writer LE(Out, llvm::endianness::little);
       uint64_t Start = Out.tell(); (void)Start;
 
-      unsigned char Flags = (Data.AlreadyIncluded << 6)
-                          | (Data.HFI.isImport << 5)
+      unsigned char Flags = (Data.HFI.isImport << 5)
                           | (Writer.isWritingStdCXXNamedModules() ? 0 :
                              Data.HFI.isPragmaOnce << 4)
                           | (Data.HFI.DirInfo << 1);
@@ -2167,6 +2173,14 @@ namespace {
         LE.write<IdentifierID>(
             Writer.getIdentifierRef(Data.HFI.LazyControllingMacro.getPtr()));
 
+      LE.write<uint32_t>(Data.Includers.size());
+      for (const Module *M : Data.Includers) {
+        if (!M)
+          LE.write<uint32_t>(0);
+        else if (uint32_t ModID = Writer.getLocalOrImportedSubmoduleID(M))
+          LE.write<uint32_t>(ModID);
+      }
+
       auto EmitModule = [&](Module *M, ModuleMap::ModuleHeaderRole Role) {
         if (uint32_t ModID = Writer.getLocalOrImportedSubmoduleID(M)) {
           uint32_t Value = (ModID << 3) | (unsigned)Role;
@@ -2238,7 +2252,7 @@ void ASTWriter::WriteHeaderSearch(const HeaderSearch &HS) {
         HeaderFileInfoTrait::key_type Key = {
             FilenameDup, *U.Size, IncludeTimestamps ? *U.ModTime : 0};
         HeaderFileInfoTrait::data_type Data = {
-            Empty, false, {}, {M, ModuleMap::headerKindToRole(U.Kind)}};
+            Empty, {}, {}, {M, ModuleMap::headerKindToRole(U.Kind)}};
         // FIXME: Deal with cases where there are multiple unresolved header
         // directives in different submodules for the same header.
         Generator.insert(Key, Data, GeneratorTrait);
@@ -2278,13 +2292,27 @@ void ASTWriter::WriteHeaderSearch(const HeaderSearch &HS) {
       SavedStrings.push_back(Filename.data());
     }
 
-    bool Included = HFI->IsLocallyIncluded || PP->alreadyIncluded(*File);
+    std::vector<const Module *> Includers;
+    if (WritingModule) {
+      llvm::DenseSet<const Module *> Seen;
+      std::function<void(const Module *)> Visit = [&](const Module *M) {
+        if (!Seen.insert(M).second)
+          return;
+        if (M->Includes.contains(*File))
+          Includers.push_back(M);
+        for (const Module *SubM : M->submodules())
+          Visit(SubM);
+      };
+      Visit(WritingModule);
+    } else if (PP->getTopLevelIncludes().contains(*File)) {
+      Includers.push_back(nullptr);
+    }
 
     HeaderFileInfoTrait::key_type Key = {
         Filename, File->getSize(),
         getTimestampForOutput(File->getModificationTime())};
     HeaderFileInfoTrait::data_type Data = {
-      *HFI, Included, HS.getModuleMap().findResolvedModulesForHeader(*File), {}
+      *HFI, Includers, HS.getModuleMap().findResolvedModulesForHeader(*File), {}
     };
     Generator.insert(Key, Data, GeneratorTrait);
     ++NumHeaderSearchEntries;
diff --git a/clang/test/Modules/per-submodule-includes.m b/clang/test/Modules/per-submodule-includes.m
new file mode 100644
index 0000000000000..39fd255e8e93e
--- /dev/null
+++ b/clang/test/Modules/per-submodule-includes.m
@@ -0,0 +1,53 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- frameworks/Textual.framework/Headers/Header.h
+static int symbol;
+
+//--- frameworks/FW.framework/Modules/module.modulemap
+framework module FW {
+  umbrella header "FW.h"
+  export *
+  module * { export * }
+}
+//--- frameworks/FW.framework/Headers/FW.h
+#import <FW/Sub1.h>
+#import <FW/Sub2.h>
+//--- frameworks/FW.framework/Headers/Sub1.h
+//--- frameworks/FW.framework/Headers/Sub2.h
+#import <Textual/Header.h>
+
+//--- pch.modulemap
+module __PCH {
+  header "pch.h"
+  export *
+}
+//--- pch.h
+#import <FW/Sub1.h>
+
+//--- tu.m
+#import <Textual/Header.h>
+int fn() { return symbol; }
+
+// Compilation using the PCH regularly succeeds. The import of FW/Sub1.h in the
+// PCH is treated textually due to -fmodule-name=FW.
+//
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fimplicit-module-maps -F %t/frameworks -fmodule-name=FW \
+// RUN:   -emit-pch -x objective-c %t/pch.h -o %t/pch.h.gch
+//
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fimplicit-module-maps -F %t/frameworks -fmodule-name=FW \
+// RUN:   -include-pch %t/pch.h.gch -fsyntax-only %t/tu.m
+
+// Compilation using the PCH as precompiled module fails. The import of FW/Sub1.h
+// in the PCH is translated to an import. Nothing is preventing that now that
+// -fmodule-name=FW has been replaced with -fmodule-name=__PCH.
+//
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fimplicit-module-maps -F %t/frameworks \
+// RUN:   -emit-module -fmodule-name=__PCH -x objective-c %t/pch.modulemap -o %t/pch.h.pcm
+//
+// Loading FW.pcm marks Textual/Header.h as imported (because it is imported in
+// FW.Sub2), so the TU does not import it again. It's contents remain invisible,
+// though.
+//
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fimplicit-module-maps -F %t/frameworks \
+// RUN:   -include %t/pch.h -fmodule-map-file=%t/pch.modulemap -fsyntax-only %t/tu.m

>From d03af159a645cd52ae55e18449f33c78e360cbdf Mon Sep 17 00:00:00 2001
From: Qiongsi Wu <qiongsi_wu at apple.com>
Date: Mon, 1 Dec 2025 14:07:07 -0800
Subject: [PATCH 2/8] Move the included vector into
 HeaderFileInfoTrait::data_type and add a test case.

---
 clang/lib/Serialization/ASTWriter.cpp         | 21 +++---
 .../Modules/import-submodule-visibility.c     | 64 +++++++++++++++++++
 2 files changed, 76 insertions(+), 9 deletions(-)
 create mode 100644 clang/test/Modules/import-submodule-visibility.c

diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index d16a97610e241..d240d5d19914a 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -2099,11 +2099,11 @@ namespace {
 
     struct data_type {
       data_type(const HeaderFileInfo &HFI,
-                const std::vector<const Module *> &Includers,
+                std::vector<const Module *> &&Includers,
                 ArrayRef<ModuleMap::KnownHeader> KnownHeaders,
                 UnresolvedModule Unresolved)
-          : HFI(HFI), Includers(Includers), KnownHeaders(KnownHeaders),
-            Unresolved(Unresolved) {}
+          : HFI(HFI), Includers(std::move(Includers)),
+            KnownHeaders(KnownHeaders), Unresolved(Unresolved) {}
 
       HeaderFileInfo HFI;
       std::vector<const Module *> Includers;
@@ -2161,10 +2161,11 @@ namespace {
       endian::Writer LE(Out, llvm::endianness::little);
       uint64_t Start = Out.tell(); (void)Start;
 
-      unsigned char Flags = (Data.HFI.isImport << 5)
-                          | (Writer.isWritingStdCXXNamedModules() ? 0 :
-                             Data.HFI.isPragmaOnce << 4)
-                          | (Data.HFI.DirInfo << 1);
+      unsigned char Flags =
+          (Data.HFI.isImport << 5) |
+          (Writer.isWritingStdCXXNamedModules() ? 0
+                                                : Data.HFI.isPragmaOnce << 4) |
+          (Data.HFI.DirInfo << 1);
       LE.write<uint8_t>(Flags);
 
       if (Data.HFI.LazyControllingMacro.isID())
@@ -2312,8 +2313,10 @@ void ASTWriter::WriteHeaderSearch(const HeaderSearch &HS) {
         Filename, File->getSize(),
         getTimestampForOutput(File->getModificationTime())};
     HeaderFileInfoTrait::data_type Data = {
-      *HFI, Includers, HS.getModuleMap().findResolvedModulesForHeader(*File), {}
-    };
+        *HFI,
+        std::move(Includers),
+        HS.getModuleMap().findResolvedModulesForHeader(*File),
+        {}};
     Generator.insert(Key, Data, GeneratorTrait);
     ++NumHeaderSearchEntries;
   }
diff --git a/clang/test/Modules/import-submodule-visibility.c b/clang/test/Modules/import-submodule-visibility.c
new file mode 100644
index 0000000000000..3491494ea7cc0
--- /dev/null
+++ b/clang/test/Modules/import-submodule-visibility.c
@@ -0,0 +1,64 @@
+// This test checks that imports of headers that appeared in a different submodule than
+// what is imported by the current TU don't affect the compilation.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- C/C.h
+#include "Textual.h"
+//--- C/module.modulemap
+module C { header "C.h" }
+
+//--- D/D1.h
+#include "Textual.h"
+//--- D/D2.h
+//--- D/module.modulemap
+module D {
+  module D1 { header "D1.h" }
+  module D2 { header "D2.h" }
+}
+
+//--- E/E1.h
+#include "E2.h"
+//--- E/E2.h
+#include "Textual.h"
+//--- E/module.modulemap
+module E {
+  module E1 { header "E1.h" }
+  module E2 { header "E2.h" }
+}
+
+//--- Textual.h
+#define MACRO_TEXTUAL 1
+
+//--- test_top.c
+#import "Textual.h"
+static int x = MACRO_TEXTUAL;
+
+//--- test_sub.c
+#import "D/D2.h"
+#import "Textual.h"
+static int x = MACRO_TEXTUAL;
+
+//--- test_transitive.c
+#import "E/E1.h"
+#import "Textual.h"
+static int x = MACRO_TEXTUAL;
+
+// Simply loading a PCM file containing top-level module including a header does
+// not prevent inclusion of that header in the TU.
+//
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/C/module.modulemap -fmodule-name=C -o %t/C.pcm
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_top.c -fmodule-file=%t/C.pcm
+
+// Loading a PCM file and importing its empty submodule does not prevent
+// inclusion of headers included by invisible sibling submodules.
+//
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/D/module.modulemap -fmodule-name=D -o %t/D.pcm
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_sub.c -fmodule-file=%t/D.pcm
+
+// Loading a PCM file and importing a submodule does not prevent inclusion of
+// headers included by some of its transitive un-exported dependencies.
+//
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/E/module.modulemap -fmodule-name=E -o %t/E.pcm
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_transitive.c -fmodule-file=%t/E.pcm

>From 984d0005f4fe367de960acd07b4d79b5ee53ade3 Mon Sep 17 00:00:00 2001
From: Qiongsi Wu <qiongsi_wu at apple.com>
Date: Sat, 13 Dec 2025 14:55:36 -0800
Subject: [PATCH 3/8] Avoid looping in Preprocessor::alreadyIncluded.

---
 clang/include/clang/Lex/Preprocessor.h | 25 +++++++++++++------------
 clang/lib/Lex/Preprocessor.cpp         |  2 ++
 clang/lib/Serialization/ASTReader.cpp  |  1 +
 3 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/clang/include/clang/Lex/Preprocessor.h b/clang/include/clang/Lex/Preprocessor.h
index 6cde2eb1fb703..437fad823ef2f 100644
--- a/clang/include/clang/Lex/Preprocessor.h
+++ b/clang/include/clang/Lex/Preprocessor.h
@@ -1052,8 +1052,10 @@ class Preprocessor {
     /// The set of modules that are visible within the submodule.
     VisibleModuleSet VisibleModules;
 
-    /// The files that have been included.
+    /// The files that have been included. This set includes the headers
+    /// that have been included and visible in this submodule.
     IncludedFilesSet IncludedFiles;
+
     // FIXME: CounterValue?
     // FIXME: PragmaPushMacroInfo?
   };
@@ -1555,26 +1557,22 @@ class Preprocessor {
     bool AlreadyIncluded = alreadyIncluded(File);
     HeaderInfo.getFileInfo(File).IsLocallyIncluded = true;
     CurSubmoduleState->IncludedFiles.insert(File);
-    if (!BuildingSubmoduleStack.empty())
-      BuildingSubmoduleStack.back().M->Includes.insert(File);
-    else if (Module *M = getCurrentModule())
+
+    Module *M = BuildingSubmoduleStack.empty()
+                    ? getCurrentModule()
+                    : BuildingSubmoduleStack.back().M;
+    if (M)
       M->Includes.insert(File);
     else
       Includes.insert(File);
+
     return !AlreadyIncluded;
   }
 
   /// Return true if this header has already been included.
   bool alreadyIncluded(FileEntryRef File) const {
     HeaderInfo.getFileInfo(File);
-    if (CurSubmoduleState->IncludedFiles.contains(File))
-      return true;
-    // TODO: Do this more efficiently.
-    for (const auto &[Name, M] : HeaderInfo.getModuleMap().modules())
-      if (CurSubmoduleState->VisibleModules.isVisible(M))
-        if (M->Includes.contains(File))
-          return true;
-    return false;
+    return CurSubmoduleState->IncludedFiles.contains(File);
   }
 
   void markIncludedOnTopLevel(const FileEntry *File) {
@@ -1584,6 +1582,9 @@ class Preprocessor {
 
   void markIncludedInModule(Module *M, const FileEntry *File) {
     M->Includes.insert(File);
+
+    if (CurSubmoduleState->VisibleModules.isVisible(M))
+      CurSubmoduleState->IncludedFiles.insert(File);
   }
 
   const IncludedFilesSet &getTopLevelIncludes() const { return Includes; }
diff --git a/clang/lib/Lex/Preprocessor.cpp b/clang/lib/Lex/Preprocessor.cpp
index b08459632aacb..47f47a9bba150 100644
--- a/clang/lib/Lex/Preprocessor.cpp
+++ b/clang/lib/Lex/Preprocessor.cpp
@@ -1448,6 +1448,8 @@ void Preprocessor::makeModuleVisible(Module *M, SourceLocation Loc,
             << Message;
       });
 
+  CurSubmoduleState->IncludedFiles.insert_range(M->Includes);
+
   // Add this module to the imports list of the currently-built submodule.
   if (!BuildingSubmoduleStack.empty() && M != BuildingSubmoduleStack.back().M)
     BuildingSubmoduleStack.back().M->Imports.insert(M);
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index bad8a2331efb5..27ba0296f668b 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -2425,6 +2425,7 @@ HeaderFileInfoTrait::ReadData(internal_key_ref key, const unsigned char *d,
     // implicit module import.
     SubmoduleID GlobalSMID = Reader.getGlobalSubmoduleID(M, LocalSMID);
     Module *Mod = Reader.getSubmodule(GlobalSMID);
+    PP.markIncludedInModule(Mod, *FE);
 
     if (FE) {
       // FIXME: NameAsWritten

>From 86e2c367d1180d0c1c4cdadd805739e552d913ba Mon Sep 17 00:00:00 2001
From: Qiongsi Wu <qiongsi_wu at apple.com>
Date: Fri, 6 Mar 2026 14:00:10 -0800
Subject: [PATCH 4/8] Adding two tests, and removing dead code.

---
 clang/include/clang/Lex/Preprocessor.h        |  13 +-
 clang/lib/Lex/Preprocessor.cpp                |   5 +-
 clang/lib/Serialization/ASTReader.cpp         |   2 -
 .../import-textual-reenter-not-visible.c      | 156 +++++++++++++++
 .../Modules/import-textual-skip-visible.c     | 186 ++++++++++++++++++
 5 files changed, 349 insertions(+), 13 deletions(-)
 create mode 100644 clang/test/Modules/import-textual-reenter-not-visible.c
 create mode 100644 clang/test/Modules/import-textual-skip-visible.c

diff --git a/clang/include/clang/Lex/Preprocessor.h b/clang/include/clang/Lex/Preprocessor.h
index 437fad823ef2f..c067dec113e60 100644
--- a/clang/include/clang/Lex/Preprocessor.h
+++ b/clang/include/clang/Lex/Preprocessor.h
@@ -1052,9 +1052,8 @@ class Preprocessor {
     /// The set of modules that are visible within the submodule.
     VisibleModuleSet VisibleModules;
 
-    /// The files that have been included. This set includes the headers
-    /// that have been included and visible in this submodule.
-    IncludedFilesSet IncludedFiles;
+    /// The files that have been included and visible within the submodule.
+    IncludedFilesSet VisibleIncludedFiles;
 
     // FIXME: CounterValue?
     // FIXME: PragmaPushMacroInfo?
@@ -1556,7 +1555,7 @@ class Preprocessor {
   bool markIncluded(FileEntryRef File) {
     bool AlreadyIncluded = alreadyIncluded(File);
     HeaderInfo.getFileInfo(File).IsLocallyIncluded = true;
-    CurSubmoduleState->IncludedFiles.insert(File);
+    CurSubmoduleState->VisibleIncludedFiles.insert(File);
 
     Module *M = BuildingSubmoduleStack.empty()
                     ? getCurrentModule()
@@ -1572,19 +1571,19 @@ class Preprocessor {
   /// Return true if this header has already been included.
   bool alreadyIncluded(FileEntryRef File) const {
     HeaderInfo.getFileInfo(File);
-    return CurSubmoduleState->IncludedFiles.contains(File);
+    return CurSubmoduleState->VisibleIncludedFiles.contains(File);
   }
 
   void markIncludedOnTopLevel(const FileEntry *File) {
     Includes.insert(File);
-    CurSubmoduleState->IncludedFiles.insert(File);
+    CurSubmoduleState->VisibleIncludedFiles.insert(File);
   }
 
   void markIncludedInModule(Module *M, const FileEntry *File) {
     M->Includes.insert(File);
 
     if (CurSubmoduleState->VisibleModules.isVisible(M))
-      CurSubmoduleState->IncludedFiles.insert(File);
+      CurSubmoduleState->VisibleIncludedFiles.insert(File);
   }
 
   const IncludedFilesSet &getTopLevelIncludes() const { return Includes; }
diff --git a/clang/lib/Lex/Preprocessor.cpp b/clang/lib/Lex/Preprocessor.cpp
index 47f47a9bba150..ae159956f331d 100644
--- a/clang/lib/Lex/Preprocessor.cpp
+++ b/clang/lib/Lex/Preprocessor.cpp
@@ -1443,13 +1443,10 @@ void Preprocessor::makeModuleVisible(Module *M, SourceLocation Loc,
         // FIXME: Include the path in the diagnostic.
         // FIXME: Include the import location for the conflicting module.
         Diag(ModuleImportLoc, diag::warn_module_conflict)
-            << Path[0]->getFullModuleName()
-            << Conflict->getFullModuleName()
+            << Path[0]->getFullModuleName() << Conflict->getFullModuleName()
             << Message;
       });
 
-  CurSubmoduleState->IncludedFiles.insert_range(M->Includes);
-
   // Add this module to the imports list of the currently-built submodule.
   if (!BuildingSubmoduleStack.empty() && M != BuildingSubmoduleStack.back().M)
     BuildingSubmoduleStack.back().M->Imports.insert(M);
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 27ba0296f668b..b3795415a8309 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -2425,8 +2425,6 @@ HeaderFileInfoTrait::ReadData(internal_key_ref key, const unsigned char *d,
     // implicit module import.
     SubmoduleID GlobalSMID = Reader.getGlobalSubmoduleID(M, LocalSMID);
     Module *Mod = Reader.getSubmodule(GlobalSMID);
-    PP.markIncludedInModule(Mod, *FE);
-
     if (FE) {
       // FIXME: NameAsWritten
       Module::Header H = {std::string(key.Filename), "", *FE};
diff --git a/clang/test/Modules/import-textual-reenter-not-visible.c b/clang/test/Modules/import-textual-reenter-not-visible.c
new file mode 100644
index 0000000000000..9254283fc43f2
--- /dev/null
+++ b/clang/test/Modules/import-textual-reenter-not-visible.c
@@ -0,0 +1,156 @@
+// Test that #import can re-enter a textual header when the header was included
+// by a module that is reachable but NOT visible. The key invariant:
+//
+//   If a textual header was included by module M, and M is NOT visible in the
+//   current TU, then #import of that header MUST re-enter the file (not skip it).
+//
+// This is the complement of import-textual-skip-visible.c, which verifies
+// the opposite: visible modules DO suppress re-entry. Here we verify that
+// non-visible modules do NOT suppress re-entry, even when the PCM containing
+// the header info has been loaded.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- Textual.h
+#define MACRO_TEXTUAL 1
+
+// =========================================================================
+// Module E: E1 includes E2, E2 includes Textual.h. NO export *.
+// =========================================================================
+//--- E/E1.h
+#include "E2.h"
+//--- E/E2.h
+#include "Textual.h"
+//--- E/module.modulemap
+module E {
+  module E1 { header "E1.h" }
+  module E2 { header "E2.h" }
+}
+
+// =========================================================================
+// Module F: F1 imports E1 (cross-module via -fmodule-file). NO export *.
+// =========================================================================
+//--- F/F1.h
+#include "E/E1.h"
+//--- F/module.modulemap
+module F {
+  module F1 { header "F1.h" }
+}
+
+// =========================================================================
+// Module G: G1 imports E1 with export *, but E1 itself does NOT export E2.
+// So importing G1 makes E1 visible (via G1's export *), but E2 is still not
+// visible because E1 has no export *.
+// =========================================================================
+//--- G/G1.h
+#include "E/E1.h"
+//--- G/module.modulemap
+module G {
+  module G1 {
+    header "G1.h"
+    export *
+  }
+}
+
+// =========================================================================
+// Module H: H1 imports E1, H2 is empty. Import H2 only — E2 is not visible.
+// =========================================================================
+//--- H/H1.h
+#include "E/E1.h"
+//--- H/H2.h
+// empty
+//--- H/module.modulemap
+module H {
+  module H1 { header "H1.h" }
+  module H2 { header "H2.h" }
+}
+
+// =========================================================================
+// Module J: another independent module that also includes Textual.h.
+// =========================================================================
+//--- J/J1.h
+#include "Textual.h"
+//--- J/module.modulemap
+module J {
+  module J1 { header "J1.h" }
+}
+
+// =========================================================================
+// Test 1: Cross-module, no export.
+// Import F1, which imports E1 (no export *). E2 included Textual.h but is
+// NOT visible. #import Textual.h must re-enter the file.
+// =========================================================================
+//--- test_cross_no_export.c
+#import "F/F1.h"
+#import "Textual.h"
+static int x = MACRO_TEXTUAL;
+
+// =========================================================================
+// Test 2: Cross-module, partial export chain.
+// Import G1 (export *) → E1 (no export *). E1 is visible but E2 is not.
+// Textual.h was included by E2 — must be re-enterable.
+// =========================================================================
+//--- test_partial_export.c
+#import "G/G1.h"
+#import "Textual.h"
+static int x = MACRO_TEXTUAL;
+
+// =========================================================================
+// Test 3: Import a sibling; the other sibling's transitive deps included
+// Textual.h. Import H2 only — H1 (and its dep E1, E2) are not visible.
+// Textual.h must be re-enterable.
+// =========================================================================
+//--- test_sibling_invisible.c
+#import "H/H2.h"
+#import "Textual.h"
+static int x = MACRO_TEXTUAL;
+
+// =========================================================================
+// Test 4: Multiple PCMs loaded, none with Textual.h visible.
+// Load both E.pcm and J.pcm. Import neither E2 nor J1.
+// Textual.h was included by both E2 and J1, but neither is visible.
+// Must be re-enterable.
+// =========================================================================
+//--- test_multiple_pcms.c
+// Don't import anything — just load PCMs via -fmodule-file.
+#import "Textual.h"
+static int x = MACRO_TEXTUAL;
+
+// =========================================================================
+// Test 5: Cross-module transitive, no export at any level.
+// Import F1 → E1 → E2 → Textual.h. Neither F1 nor E1 has export *.
+// E2 is not visible. Textual.h must be re-enterable.
+// =========================================================================
+//--- test_deep_no_export.c
+#import "F/F1.h"
+#import "Textual.h"
+static int x = MACRO_TEXTUAL;
+
+// =========================================================================
+// Build PCMs.
+// =========================================================================
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/E/module.modulemap -fmodule-name=E -o %t/E.pcm
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/F/module.modulemap -fmodule-name=F -fmodule-file=%t/E.pcm -o %t/F.pcm
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/G/module.modulemap -fmodule-name=G -fmodule-file=%t/E.pcm -o %t/G.pcm
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/H/module.modulemap -fmodule-name=H -fmodule-file=%t/E.pcm -o %t/H.pcm
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/J/module.modulemap -fmodule-name=J -o %t/J.pcm
+
+// =========================================================================
+// Run tests.
+// =========================================================================
+
+// Test 1: cross-module, no export — Textual.h must be re-entered.
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_cross_no_export.c -fmodule-file=%t/F.pcm
+
+// Test 2: partial export chain — Textual.h must be re-entered.
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_partial_export.c -fmodule-file=%t/G.pcm
+
+// Test 3: sibling invisible — Textual.h must be re-entered.
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_sibling_invisible.c -fmodule-file=%t/H.pcm
+
+// Test 4: multiple PCMs, none visible — Textual.h must be re-entered.
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_multiple_pcms.c -fmodule-file=%t/E.pcm -fmodule-file=%t/J.pcm
+
+// Test 5: deep transitive, no export — Textual.h must be re-entered.
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_deep_no_export.c -fmodule-file=%t/F.pcm
diff --git a/clang/test/Modules/import-textual-skip-visible.c b/clang/test/Modules/import-textual-skip-visible.c
new file mode 100644
index 0000000000000..c107472128dbd
--- /dev/null
+++ b/clang/test/Modules/import-textual-skip-visible.c
@@ -0,0 +1,186 @@
+// Test that #import does NOT re-enter a textual header when the header was
+// included by a module that IS visible. The key invariant:
+//
+//   If a textual header was included by module M, and M is visible in the
+//   current TU (directly or via transitive export), then #import of that
+//   header MUST be skipped (the file is already included).
+//
+// This is the complement of import-textual-reenter-not-visible.c, which
+// verifies the opposite: non-visible modules do NOT suppress re-entry.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// Textual.h has no include guard. If #import incorrectly re-enters the file,
+// the duplicate typedef will produce a compilation error.
+//--- Textual.h
+typedef int textual_type_t;
+#define MACRO_TEXTUAL 1
+
+// =========================================================================
+// Module E: E1 includes E2, E2 includes Textual.h. E1 has export *.
+// =========================================================================
+//--- E/E1.h
+#include "E2.h"
+//--- E/E2.h
+#include "Textual.h"
+//--- E/module.modulemap
+module E {
+  module E1 {
+    header "E1.h"
+    export *
+  }
+  module E2 { header "E2.h" }
+}
+
+// =========================================================================
+// Module F: F1 imports E1 cross-module. F1 has export *.
+// =========================================================================
+//--- F/F1.h
+#include "E/E1.h"
+//--- F/module.modulemap
+module F {
+  module F1 {
+    header "F1.h"
+    export *
+  }
+}
+
+// =========================================================================
+// Module G: G1 imports F1 cross-module. G1 has export *.
+// =========================================================================
+//--- G/G1.h
+#include "F/F1.h"
+//--- G/module.modulemap
+module G {
+  module G1 {
+    header "G1.h"
+    export *
+  }
+}
+
+// =========================================================================
+// Module A: A1 independently includes Textual.h (for early deserialization).
+// =========================================================================
+//--- A/A1.h
+#include "Textual.h"
+//--- A/module.modulemap
+module A {
+  module A1 { header "A1.h" }
+}
+
+// =========================================================================
+// Test 1: Sibling re-export within a single module.
+// Import E1 (export *) → E2 becomes visible → Textual.h's macro should
+// be available as a module macro without an explicit #import.
+// =========================================================================
+//--- test_sibling_reexport.c
+#import "E/E1.h"
+#ifdef MACRO_TEXTUAL
+textual_type_t val = MACRO_TEXTUAL;
+#else
+#error "MACRO_TEXTUAL should be visible via E1's re-export of E2"
+#endif
+
+// =========================================================================
+// Test 2: Cross-module transitive re-export (depth 2).
+// Import F1 (export *) → E1 (export *) → E2 visible. Macro should be
+// available without explicit #import.
+// =========================================================================
+//--- test_cross_depth2.c
+#import "F/F1.h"
+#ifdef MACRO_TEXTUAL
+textual_type_t val = MACRO_TEXTUAL;
+#else
+#error "MACRO_TEXTUAL should be visible via F1 -> E1 -> E2 export chain"
+#endif
+
+// =========================================================================
+// Test 3: Cross-module transitive re-export (depth 3).
+// Import G1 (export *) → F1 (export *) → E1 (export *) → E2 visible.
+// =========================================================================
+//--- test_cross_depth3.c
+#import "G/G1.h"
+#ifdef MACRO_TEXTUAL
+textual_type_t val = MACRO_TEXTUAL;
+#else
+#error "MACRO_TEXTUAL should be visible via G1 -> F1 -> E1 -> E2 export chain"
+#endif
+
+// =========================================================================
+// Test 4: Explicit #import of already-visible textual header is suppressed.
+// Import E1 (export *) → E2 visible → Textual.h already included.
+// A subsequent #import Textual.h should be a no-op. The typedef in
+// Textual.h (no include guard) would cause a duplicate definition error
+// if the file were incorrectly re-entered.
+// =========================================================================
+//--- test_import_suppressed.c
+#import "E/E1.h"
+#import "Textual.h"
+textual_type_t val = MACRO_TEXTUAL;
+
+// =========================================================================
+// Test 5: Cross-module #import suppression (depth 2).
+// Import F1 (export * chain) → E2 visible. Explicit #import of Textual.h
+// must be suppressed.
+// =========================================================================
+//--- test_import_suppressed_depth2.c
+#import "F/F1.h"
+#import "Textual.h"
+textual_type_t val = MACRO_TEXTUAL;
+
+// =========================================================================
+// Test 6: Cross-module #import suppression (depth 3).
+// Import G1 (export * chain) → E2 visible. Explicit #import of Textual.h
+// must be suppressed.
+// =========================================================================
+//--- test_import_suppressed_depth3.c
+#import "G/G1.h"
+#import "Textual.h"
+textual_type_t val = MACRO_TEXTUAL;
+
+// =========================================================================
+// Test 7: Early deserialization — import a different module that also
+// includes Textual.h BEFORE importing E1. This forces Textual.h's header
+// info to be deserialized (via markIncludedInModule) at a time when E2 is
+// NOT yet visible. Then import E1 (export *) → E2 becomes visible.
+// The subsequent #import Textual.h must still be suppressed.
+// =========================================================================
+//--- test_early_deser.c
+#import "A/A1.h"
+#import "E/E1.h"
+#import "Textual.h"
+textual_type_t val = MACRO_TEXTUAL;
+
+// =========================================================================
+// Build PCMs.
+// =========================================================================
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/E/module.modulemap -fmodule-name=E -o %t/E.pcm
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/F/module.modulemap -fmodule-name=F -fmodule-file=%t/E.pcm -o %t/F.pcm
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/G/module.modulemap -fmodule-name=G -fmodule-file=%t/F.pcm -o %t/G.pcm
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/A/module.modulemap -fmodule-name=A -o %t/A.pcm
+
+// =========================================================================
+// Run tests.
+// =========================================================================
+
+// Test 1: sibling re-export — macro visible without #import.
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_sibling_reexport.c -fmodule-file=%t/E.pcm
+
+// Test 2: depth-2 cross-module re-export — macro visible.
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_cross_depth2.c -fmodule-file=%t/F.pcm
+
+// Test 3: depth-3 cross-module re-export — macro visible.
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_cross_depth3.c -fmodule-file=%t/G.pcm
+
+// Test 4: explicit #import suppressed (single module).
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_import_suppressed.c -fmodule-file=%t/E.pcm
+
+// Test 5: explicit #import suppressed (depth 2).
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_import_suppressed_depth2.c -fmodule-file=%t/F.pcm
+
+// Test 6: explicit #import suppressed (depth 3).
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_import_suppressed_depth3.c -fmodule-file=%t/G.pcm
+
+// Test 7: early deserialization — #import suppressed despite prior deser.
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_early_deser.c -fmodule-file=%t/A.pcm -fmodule-file=%t/E.pcm

>From ae303fc268b3b4858c8d7458055822b2343f8d55 Mon Sep 17 00:00:00 2001
From: Qiongsi Wu <qiongsi_wu at apple.com>
Date: Tue, 10 Mar 2026 10:29:39 -0700
Subject: [PATCH 5/8] Adding three tests where we import multiple textual
 headers across different modules.

---
 .../Modules/import-textual-mixed-visibility.c | 93 +++++++++++++++++++
 .../import-textual-reenter-multi-layer.c      | 75 +++++++++++++++
 .../Modules/import-textual-skip-multi-layer.c | 90 ++++++++++++++++++
 3 files changed, 258 insertions(+)
 create mode 100644 clang/test/Modules/import-textual-mixed-visibility.c
 create mode 100644 clang/test/Modules/import-textual-reenter-multi-layer.c
 create mode 100644 clang/test/Modules/import-textual-skip-multi-layer.c

diff --git a/clang/test/Modules/import-textual-mixed-visibility.c b/clang/test/Modules/import-textual-mixed-visibility.c
new file mode 100644
index 0000000000000..28cfa25b82eaa
--- /dev/null
+++ b/clang/test/Modules/import-textual-mixed-visibility.c
@@ -0,0 +1,93 @@
+// Test mixed visibility: one textual header is visible (via export *) and
+// another is not. The visible one must be suppressed on #import, and the
+// non-visible one must be re-entered.
+//
+// Setup:
+//   Module A: A1 textually includes T1.h (export *)
+//   Module B: B1 includes B2 (NO export *), B2 textually includes T2.h
+//   Module C: C1 imports A1 and B1 cross-module (export *)
+//
+// The TU imports C1. Because C1 has export *:
+//   - A1 is visible (C1 re-exports A1, and A1 has export *)
+//   - B1 is visible (C1 re-exports B1)
+//   - B2 is NOT visible (B1 has no export *, so B2 is not re-exported)
+//
+// Result:
+//   T1.h included by A1 (visible): suppress
+//   T2.h included by B2 (NOT visible): re-enter
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// T1.h has a typedef — if incorrectly re-entered, duplicate typedef errors.
+//--- T1.h
+typedef int t1_type;
+#define MACRO_T1 10
+
+// T2.h has only a macro — if incorrectly suppressed, MACRO_T2 is undefined.
+//--- T2.h
+#define MACRO_T2 20
+
+// =========================================================================
+// Module A: A1 textually includes T1.h. Has export *.
+// =========================================================================
+//--- A/A1.h
+#include "T1.h"
+//--- A/module.modulemap
+module A {
+  module A1 {
+    header "A1.h"
+    export *
+  }
+}
+
+// =========================================================================
+// Module B: B1 includes B2, B2 textually includes T2.h. B1 has NO export *.
+// =========================================================================
+//--- B/B1.h
+#include "B2.h"
+//--- B/B2.h
+#include "T2.h"
+//--- B/module.modulemap
+module B {
+  module B1 { header "B1.h" }
+  module B2 { header "B2.h" }
+}
+
+// =========================================================================
+// Module C: C1 imports A1 and B1 cross-module. Has export *.
+// =========================================================================
+//--- C/C1.h
+#include "A/A1.h"
+#include "B/B1.h"
+//--- C/module.modulemap
+module C {
+  module C1 {
+    header "C1.h"
+    export *
+  }
+}
+
+// =========================================================================
+// Test: Import C1 (export *) → A1 (export *) visible, B1 visible but B2 not.
+// T1.h included by A1 (visible) → suppress (typedef would error if re-entered).
+// T2.h included by B2 (NOT visible) → re-enter (macro must be defined).
+// =========================================================================
+//--- test.c
+#import "C/C1.h"
+#import "T1.h"
+#import "T2.h"
+t1_type x = MACRO_T1;
+static int y = MACRO_T2;
+
+// =========================================================================
+// Build PCMs.
+// =========================================================================
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/A/module.modulemap -fmodule-name=A -o %t/A.pcm
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/B/module.modulemap -fmodule-name=B -o %t/B.pcm
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/C/module.modulemap -fmodule-name=C -fmodule-file=%t/A.pcm -fmodule-file=%t/B.pcm -o %t/C.pcm
+
+// =========================================================================
+// Run test.
+// =========================================================================
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test.c -fmodule-file=%t/C.pcm
diff --git a/clang/test/Modules/import-textual-reenter-multi-layer.c b/clang/test/Modules/import-textual-reenter-multi-layer.c
new file mode 100644
index 0000000000000..3cf4cf21093f0
--- /dev/null
+++ b/clang/test/Modules/import-textual-reenter-multi-layer.c
@@ -0,0 +1,75 @@
+// Test that #import re-enters textual headers included at multiple layers of
+// a cross-module dependency chain when none of the including modules are
+// visible.
+//
+// Setup:
+//   Module A: A1 textually includes T1.h (no export *)
+//   Module B: B1 imports A1 cross-module and textually includes T2.h (no export *)
+//   Module C: C1 imports B1 cross-module (no export *)
+//
+// The TU imports C1, then #import T1.h and T2.h. Since no module in the chain
+// has export *, neither A1 nor B1 is visible, so both T1.h and T2.h must be
+// re-entered.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- T1.h
+#define MACRO_T1 10
+
+//--- T2.h
+#define MACRO_T2 20
+
+// =========================================================================
+// Module A: A1 textually includes T1.h. NO export *.
+// =========================================================================
+//--- A/A1.h
+#include "T1.h"
+//--- A/module.modulemap
+module A {
+  module A1 { header "A1.h" }
+}
+
+// =========================================================================
+// Module B: B1 imports A1 (cross-module) and textually includes T2.h.
+// NO export *.
+// =========================================================================
+//--- B/B1.h
+#include "A/A1.h"
+#include "T2.h"
+//--- B/module.modulemap
+module B {
+  module B1 { header "B1.h" }
+}
+
+// =========================================================================
+// Module C: C1 imports B1 (cross-module). NO export *.
+// =========================================================================
+//--- C/C1.h
+#include "B/B1.h"
+//--- C/module.modulemap
+module C {
+  module C1 { header "C1.h" }
+}
+
+// =========================================================================
+// Test: Import C1 → B1 → A1. No export * anywhere.
+// Neither A1 nor B1 is visible, so both T1.h and T2.h must be re-entered.
+// =========================================================================
+//--- test.c
+#import "C/C1.h"
+#import "T1.h"
+#import "T2.h"
+static int x = MACRO_T1 + MACRO_T2;
+
+// =========================================================================
+// Build PCMs.
+// =========================================================================
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/A/module.modulemap -fmodule-name=A -o %t/A.pcm
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/B/module.modulemap -fmodule-name=B -fmodule-file=%t/A.pcm -o %t/B.pcm
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/C/module.modulemap -fmodule-name=C -fmodule-file=%t/B.pcm -o %t/C.pcm
+
+// =========================================================================
+// Run test.
+// =========================================================================
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test.c -fmodule-file=%t/C.pcm
diff --git a/clang/test/Modules/import-textual-skip-multi-layer.c b/clang/test/Modules/import-textual-skip-multi-layer.c
new file mode 100644
index 0000000000000..888911e06f914
--- /dev/null
+++ b/clang/test/Modules/import-textual-skip-multi-layer.c
@@ -0,0 +1,90 @@
+// Test that #import does NOT re-enter textual headers included at multiple
+// layers of a cross-module dependency chain when the including modules ARE
+// visible (via transitive export *).
+//
+// Setup:
+//   Module A: A1 textually includes T1.h (export *)
+//   Module B: B1 imports A1 cross-module and textually includes T2.h (export *)
+//   Module C: C1 imports B1 cross-module (export *)
+//
+// The TU imports C1. Because every module in the chain has export *, B1 and
+// A1 are transitively visible. T1.h was included by A1 and T2.h was included
+// by B1 — both visible. So #import of T1.h and T2.h must be suppressed.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- T1.h
+typedef int t1_type;
+#define MACRO_T1 10
+
+//--- T2.h
+typedef int t2_type;
+#define MACRO_T2 20
+
+// =========================================================================
+// Module A: A1 textually includes T1.h. Has export *.
+// =========================================================================
+//--- A/A1.h
+#include "T1.h"
+//--- A/module.modulemap
+module A {
+  module A1 {
+    header "A1.h"
+    export *
+  }
+}
+
+// =========================================================================
+// Module B: B1 imports A1 (cross-module) and textually includes T2.h.
+// Has export *.
+// =========================================================================
+//--- B/B1.h
+#include "A/A1.h"
+#include "T2.h"
+//--- B/module.modulemap
+module B {
+  module B1 {
+    header "B1.h"
+    export *
+  }
+}
+
+// =========================================================================
+// Module C: C1 imports B1 (cross-module). Has export *.
+// =========================================================================
+//--- C/C1.h
+#include "B/B1.h"
+//--- C/module.modulemap
+module C {
+  module C1 {
+    header "C1.h"
+    export *
+  }
+}
+
+// =========================================================================
+// Test: Import C1 (export *) → B1 (export *) → A1 (export *).
+// A1 and B1 are both visible. T1.h and T2.h were included by visible
+// modules, so #import must be suppressed. The typedefs in T1.h and T2.h
+// (no include guard) would cause duplicate definition errors if the files
+// were incorrectly re-entered.
+// =========================================================================
+//--- test.c
+#import "C/C1.h"
+#import "T1.h"
+#import "T2.h"
+t1_type x = MACRO_T1;
+t2_type y = MACRO_T2;
+
+// =========================================================================
+// Build PCMs.
+// =========================================================================
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/A/module.modulemap -fmodule-name=A -o %t/A.pcm
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/B/module.modulemap -fmodule-name=B -fmodule-file=%t/A.pcm -o %t/B.pcm
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/C/module.modulemap -fmodule-name=C -fmodule-file=%t/B.pcm -o %t/C.pcm
+
+// =========================================================================
+// Run test.
+// =========================================================================
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test.c -fmodule-file=%t/C.pcm

>From 72345a54ef523b936793d016e73730a8a226402c Mon Sep 17 00:00:00 2001
From: Qiongsi Wu <qiongsi_wu at apple.com>
Date: Fri, 24 Apr 2026 14:55:08 -0700
Subject: [PATCH 6/8] Simplify testing and adding some comments. Consolidate
 key test cases in two files, one of which covers the cases we re-enter, the
 other covers the cases we do not re-enter.

---
 clang/lib/Serialization/ASTReader.cpp         |   7 +-
 .../test/Modules/import-nonmodular-reenter.c  |  71 +++++++
 clang/test/Modules/import-nonmodular-skip.c   |  63 ++++++
 .../Modules/import-submodule-visibility.c     |  64 ------
 .../Modules/import-textual-mixed-visibility.c |  93 ---------
 .../import-textual-reenter-multi-layer.c      |  75 -------
 .../import-textual-reenter-not-visible.c      | 156 ---------------
 .../Modules/import-textual-skip-multi-layer.c |  90 ---------
 .../Modules/import-textual-skip-visible.c     | 186 ------------------
 9 files changed, 140 insertions(+), 665 deletions(-)
 create mode 100644 clang/test/Modules/import-nonmodular-reenter.c
 create mode 100644 clang/test/Modules/import-nonmodular-skip.c
 delete mode 100644 clang/test/Modules/import-submodule-visibility.c
 delete mode 100644 clang/test/Modules/import-textual-mixed-visibility.c
 delete mode 100644 clang/test/Modules/import-textual-reenter-multi-layer.c
 delete mode 100644 clang/test/Modules/import-textual-reenter-not-visible.c
 delete mode 100644 clang/test/Modules/import-textual-skip-multi-layer.c
 delete mode 100644 clang/test/Modules/import-textual-skip-visible.c

diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index b3795415a8309..c17f5c89d381a 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -2401,8 +2401,13 @@ HeaderFileInfoTrait::ReadData(internal_key_ref key, const unsigned char *d,
   for (unsigned I = 0; I < IncludedCount; ++I) {
     uint32_t LocalSMID =
         endian::readNext<uint32_t, llvm::endianness::little, unaligned>(d);
-    if (!FE)
+
+    if (!FE) {
+      // We skip a header if it cannot be found on disk, since in this case
+      // it cannot be included or imported from a translation unit. Importing
+      // such a header will cause a file not found error.
       continue;
+    }
 
     if (LocalSMID == 0) {
       PP.markIncludedOnTopLevel(*FE);
diff --git a/clang/test/Modules/import-nonmodular-reenter.c b/clang/test/Modules/import-nonmodular-reenter.c
new file mode 100644
index 0000000000000..16a43133f7dea
--- /dev/null
+++ b/clang/test/Modules/import-nonmodular-reenter.c
@@ -0,0 +1,71 @@
+// Test that #import can re-enter a non-modular header when the header was included
+// by a module that is reachable but NOT visible. The key invariant:
+//
+//   If a non-modular header was included by module M, and M is NOT visible in the
+//   current TU, then #import of that header re-enters the header (not skip it).
+//
+// We cover three different cases, and we re-enter the header in the
+// translation unit for all of them:
+// 1. We load a pcm file of a module that includes the non-modular header. The
+//    module is not imported, hence is invisible.
+// 2. We import a sibling submodule of a submodule that includes the non-modular
+//    header.
+// 3. We import a submodule whose sibling transitively imports a module that
+//    includes the non-modular header.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- include/NonModular.h
+#define MACRO_NON_MODULAR 1
+
+// module A's submodule A1 includes the non-modular header NonModular.h
+//--- include/A/module.modulemap
+module A {
+  module A1 { header "A1.h" }
+  module A2 { header "A2.h" }
+}
+
+//--- include/A/A1.h
+#include "NonModular.h"
+
+//--- include/A/A2.h
+
+// module B transitively imports module A through header B1.h.
+//--- include/B/module.modulemap
+module B {
+  module B1 { header "B1.h" }
+  module B2 { header "B2.h" }
+}
+
+//--- include/B/B1.h
+#include "A/A1.h"
+
+//--- include/B/B2.h
+
+//--- test_pcm_loaded.c
+#import "NonModular.h"
+static int x = MACRO_NON_MODULAR;
+
+//--- test_invisible_sibling.c
+#import "A/A2.h"
+#import "NonModular.h"
+static int x = MACRO_NON_MODULAR;
+
+//--- test_invisible_transitive.c
+#import "B/B2.h"
+#import "NonModular.h"
+static int x = MACRO_NON_MODULAR;
+
+// Build the pcms
+// RUN: %clang_cc1 -fmodules -I %t/include -emit-module %t/include/A/module.modulemap -fmodule-name=A -o %t/A.pcm
+// RUN: %clang_cc1 -fmodules -I %t/include -emit-module %t/include/B/module.modulemap -fmodule-name=B -o %t/B.pcm
+
+// Test case 1: loading the pcm but not importing it in the TU.
+// RUN: %clang_cc1 -fmodules -I %t/include -fsyntax-only %t/test_pcm_loaded.c -fmodule-file=%t/A.pcm
+
+// Test case 2: invisible sibling.
+// RUN: %clang_cc1 -fmodules -I %t/include -fsyntax-only %t/test_invisible_sibling.c -fmodule-file=%t/A.pcm
+
+// Test case 3: invisible transtivie imported module.
+// RUN: %clang_cc1 -fmodules -I %t/include -fsyntax-only %t/test_invisible_transitive.c -fmodule-file=%t/B.pcm
diff --git a/clang/test/Modules/import-nonmodular-skip.c b/clang/test/Modules/import-nonmodular-skip.c
new file mode 100644
index 0000000000000..e9711a7fb353a
--- /dev/null
+++ b/clang/test/Modules/import-nonmodular-skip.c
@@ -0,0 +1,63 @@
+// Test that #import skips a non-modular header when the header is included
+// by a submodule that is visible. The key invariant:
+//
+//   If a non-modular header was included by module M, and M is visible in the
+//   current TU, then #import of that header skips importing the header. In
+//   other words, the #import sematic (import only once) is satisfied.
+//
+// We cover two different cases, and we skip the header in the
+// translation unit for both of them:
+// 1. We import a submodule that includes the non-modular header.
+// 2. We import a submodule that transitively imports a visible submodule
+//    that includes the non-modular header.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- include/NonModular.h
+typedef int non_modular_type_t;
+#define MACRO_NON_MODULAR 1
+
+// module A's submodule A1 includes the non-modular header NonModular.h
+//--- include/A/module.modulemap
+module A {
+  module A1 { header "A1.h" }
+  module A2 { header "A2.h" }
+}
+
+//--- include/A/A1.h
+#include "NonModular.h"
+
+//--- include/A/A2.h
+
+// module B transitively imports module A through header B1.h.
+//--- include/B/module.modulemap
+module B {
+  module B1 { header "B1.h" export * }
+  module B2 { header "B2.h" }
+}
+
+//--- include/B/B1.h
+#include "A/A1.h"
+
+//--- include/B/B2.h
+
+//--- test_direct_import.c
+#import "A/A1.h"
+#import "NonModular.h"
+non_modular_type_t val = MACRO_NON_MODULAR;
+
+//--- test_transitive_import.c
+#import "B/B1.h"
+#import "NonModular.h"
+non_modular_type_t val = MACRO_NON_MODULAR;
+
+// Build the pcms
+// RUN: %clang_cc1 -fmodules -I %t/include -emit-module %t/include/A/module.modulemap -fmodule-name=A -o %t/A.pcm
+// RUN: %clang_cc1 -fmodules -I %t/include -emit-module %t/include/B/module.modulemap -fmodule-name=B -o %t/B.pcm
+
+// Test 1: directly importing a submodule that includes a non-modular header.
+// RUN: %clang_cc1 -fmodules -I %t/include -fsyntax-only %t/test_direct_import.c -fmodule-file=%t/A.pcm
+
+// Test 2: importing a submodule that transitively imports a visible submodule that includes a non-modular header.
+// RUN: %clang_cc1 -fmodules -I %t/include -fsyntax-only %t/test_transitive_import.c -fmodule-file=%t/B.pcm
diff --git a/clang/test/Modules/import-submodule-visibility.c b/clang/test/Modules/import-submodule-visibility.c
deleted file mode 100644
index 3491494ea7cc0..0000000000000
--- a/clang/test/Modules/import-submodule-visibility.c
+++ /dev/null
@@ -1,64 +0,0 @@
-// This test checks that imports of headers that appeared in a different submodule than
-// what is imported by the current TU don't affect the compilation.
-
-// RUN: rm -rf %t
-// RUN: split-file %s %t
-
-//--- C/C.h
-#include "Textual.h"
-//--- C/module.modulemap
-module C { header "C.h" }
-
-//--- D/D1.h
-#include "Textual.h"
-//--- D/D2.h
-//--- D/module.modulemap
-module D {
-  module D1 { header "D1.h" }
-  module D2 { header "D2.h" }
-}
-
-//--- E/E1.h
-#include "E2.h"
-//--- E/E2.h
-#include "Textual.h"
-//--- E/module.modulemap
-module E {
-  module E1 { header "E1.h" }
-  module E2 { header "E2.h" }
-}
-
-//--- Textual.h
-#define MACRO_TEXTUAL 1
-
-//--- test_top.c
-#import "Textual.h"
-static int x = MACRO_TEXTUAL;
-
-//--- test_sub.c
-#import "D/D2.h"
-#import "Textual.h"
-static int x = MACRO_TEXTUAL;
-
-//--- test_transitive.c
-#import "E/E1.h"
-#import "Textual.h"
-static int x = MACRO_TEXTUAL;
-
-// Simply loading a PCM file containing top-level module including a header does
-// not prevent inclusion of that header in the TU.
-//
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/C/module.modulemap -fmodule-name=C -o %t/C.pcm
-// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_top.c -fmodule-file=%t/C.pcm
-
-// Loading a PCM file and importing its empty submodule does not prevent
-// inclusion of headers included by invisible sibling submodules.
-//
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/D/module.modulemap -fmodule-name=D -o %t/D.pcm
-// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_sub.c -fmodule-file=%t/D.pcm
-
-// Loading a PCM file and importing a submodule does not prevent inclusion of
-// headers included by some of its transitive un-exported dependencies.
-//
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/E/module.modulemap -fmodule-name=E -o %t/E.pcm
-// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_transitive.c -fmodule-file=%t/E.pcm
diff --git a/clang/test/Modules/import-textual-mixed-visibility.c b/clang/test/Modules/import-textual-mixed-visibility.c
deleted file mode 100644
index 28cfa25b82eaa..0000000000000
--- a/clang/test/Modules/import-textual-mixed-visibility.c
+++ /dev/null
@@ -1,93 +0,0 @@
-// Test mixed visibility: one textual header is visible (via export *) and
-// another is not. The visible one must be suppressed on #import, and the
-// non-visible one must be re-entered.
-//
-// Setup:
-//   Module A: A1 textually includes T1.h (export *)
-//   Module B: B1 includes B2 (NO export *), B2 textually includes T2.h
-//   Module C: C1 imports A1 and B1 cross-module (export *)
-//
-// The TU imports C1. Because C1 has export *:
-//   - A1 is visible (C1 re-exports A1, and A1 has export *)
-//   - B1 is visible (C1 re-exports B1)
-//   - B2 is NOT visible (B1 has no export *, so B2 is not re-exported)
-//
-// Result:
-//   T1.h included by A1 (visible): suppress
-//   T2.h included by B2 (NOT visible): re-enter
-
-// RUN: rm -rf %t
-// RUN: split-file %s %t
-
-// T1.h has a typedef — if incorrectly re-entered, duplicate typedef errors.
-//--- T1.h
-typedef int t1_type;
-#define MACRO_T1 10
-
-// T2.h has only a macro — if incorrectly suppressed, MACRO_T2 is undefined.
-//--- T2.h
-#define MACRO_T2 20
-
-// =========================================================================
-// Module A: A1 textually includes T1.h. Has export *.
-// =========================================================================
-//--- A/A1.h
-#include "T1.h"
-//--- A/module.modulemap
-module A {
-  module A1 {
-    header "A1.h"
-    export *
-  }
-}
-
-// =========================================================================
-// Module B: B1 includes B2, B2 textually includes T2.h. B1 has NO export *.
-// =========================================================================
-//--- B/B1.h
-#include "B2.h"
-//--- B/B2.h
-#include "T2.h"
-//--- B/module.modulemap
-module B {
-  module B1 { header "B1.h" }
-  module B2 { header "B2.h" }
-}
-
-// =========================================================================
-// Module C: C1 imports A1 and B1 cross-module. Has export *.
-// =========================================================================
-//--- C/C1.h
-#include "A/A1.h"
-#include "B/B1.h"
-//--- C/module.modulemap
-module C {
-  module C1 {
-    header "C1.h"
-    export *
-  }
-}
-
-// =========================================================================
-// Test: Import C1 (export *) → A1 (export *) visible, B1 visible but B2 not.
-// T1.h included by A1 (visible) → suppress (typedef would error if re-entered).
-// T2.h included by B2 (NOT visible) → re-enter (macro must be defined).
-// =========================================================================
-//--- test.c
-#import "C/C1.h"
-#import "T1.h"
-#import "T2.h"
-t1_type x = MACRO_T1;
-static int y = MACRO_T2;
-
-// =========================================================================
-// Build PCMs.
-// =========================================================================
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/A/module.modulemap -fmodule-name=A -o %t/A.pcm
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/B/module.modulemap -fmodule-name=B -o %t/B.pcm
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/C/module.modulemap -fmodule-name=C -fmodule-file=%t/A.pcm -fmodule-file=%t/B.pcm -o %t/C.pcm
-
-// =========================================================================
-// Run test.
-// =========================================================================
-// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test.c -fmodule-file=%t/C.pcm
diff --git a/clang/test/Modules/import-textual-reenter-multi-layer.c b/clang/test/Modules/import-textual-reenter-multi-layer.c
deleted file mode 100644
index 3cf4cf21093f0..0000000000000
--- a/clang/test/Modules/import-textual-reenter-multi-layer.c
+++ /dev/null
@@ -1,75 +0,0 @@
-// Test that #import re-enters textual headers included at multiple layers of
-// a cross-module dependency chain when none of the including modules are
-// visible.
-//
-// Setup:
-//   Module A: A1 textually includes T1.h (no export *)
-//   Module B: B1 imports A1 cross-module and textually includes T2.h (no export *)
-//   Module C: C1 imports B1 cross-module (no export *)
-//
-// The TU imports C1, then #import T1.h and T2.h. Since no module in the chain
-// has export *, neither A1 nor B1 is visible, so both T1.h and T2.h must be
-// re-entered.
-
-// RUN: rm -rf %t
-// RUN: split-file %s %t
-
-//--- T1.h
-#define MACRO_T1 10
-
-//--- T2.h
-#define MACRO_T2 20
-
-// =========================================================================
-// Module A: A1 textually includes T1.h. NO export *.
-// =========================================================================
-//--- A/A1.h
-#include "T1.h"
-//--- A/module.modulemap
-module A {
-  module A1 { header "A1.h" }
-}
-
-// =========================================================================
-// Module B: B1 imports A1 (cross-module) and textually includes T2.h.
-// NO export *.
-// =========================================================================
-//--- B/B1.h
-#include "A/A1.h"
-#include "T2.h"
-//--- B/module.modulemap
-module B {
-  module B1 { header "B1.h" }
-}
-
-// =========================================================================
-// Module C: C1 imports B1 (cross-module). NO export *.
-// =========================================================================
-//--- C/C1.h
-#include "B/B1.h"
-//--- C/module.modulemap
-module C {
-  module C1 { header "C1.h" }
-}
-
-// =========================================================================
-// Test: Import C1 → B1 → A1. No export * anywhere.
-// Neither A1 nor B1 is visible, so both T1.h and T2.h must be re-entered.
-// =========================================================================
-//--- test.c
-#import "C/C1.h"
-#import "T1.h"
-#import "T2.h"
-static int x = MACRO_T1 + MACRO_T2;
-
-// =========================================================================
-// Build PCMs.
-// =========================================================================
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/A/module.modulemap -fmodule-name=A -o %t/A.pcm
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/B/module.modulemap -fmodule-name=B -fmodule-file=%t/A.pcm -o %t/B.pcm
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/C/module.modulemap -fmodule-name=C -fmodule-file=%t/B.pcm -o %t/C.pcm
-
-// =========================================================================
-// Run test.
-// =========================================================================
-// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test.c -fmodule-file=%t/C.pcm
diff --git a/clang/test/Modules/import-textual-reenter-not-visible.c b/clang/test/Modules/import-textual-reenter-not-visible.c
deleted file mode 100644
index 9254283fc43f2..0000000000000
--- a/clang/test/Modules/import-textual-reenter-not-visible.c
+++ /dev/null
@@ -1,156 +0,0 @@
-// Test that #import can re-enter a textual header when the header was included
-// by a module that is reachable but NOT visible. The key invariant:
-//
-//   If a textual header was included by module M, and M is NOT visible in the
-//   current TU, then #import of that header MUST re-enter the file (not skip it).
-//
-// This is the complement of import-textual-skip-visible.c, which verifies
-// the opposite: visible modules DO suppress re-entry. Here we verify that
-// non-visible modules do NOT suppress re-entry, even when the PCM containing
-// the header info has been loaded.
-
-// RUN: rm -rf %t
-// RUN: split-file %s %t
-
-//--- Textual.h
-#define MACRO_TEXTUAL 1
-
-// =========================================================================
-// Module E: E1 includes E2, E2 includes Textual.h. NO export *.
-// =========================================================================
-//--- E/E1.h
-#include "E2.h"
-//--- E/E2.h
-#include "Textual.h"
-//--- E/module.modulemap
-module E {
-  module E1 { header "E1.h" }
-  module E2 { header "E2.h" }
-}
-
-// =========================================================================
-// Module F: F1 imports E1 (cross-module via -fmodule-file). NO export *.
-// =========================================================================
-//--- F/F1.h
-#include "E/E1.h"
-//--- F/module.modulemap
-module F {
-  module F1 { header "F1.h" }
-}
-
-// =========================================================================
-// Module G: G1 imports E1 with export *, but E1 itself does NOT export E2.
-// So importing G1 makes E1 visible (via G1's export *), but E2 is still not
-// visible because E1 has no export *.
-// =========================================================================
-//--- G/G1.h
-#include "E/E1.h"
-//--- G/module.modulemap
-module G {
-  module G1 {
-    header "G1.h"
-    export *
-  }
-}
-
-// =========================================================================
-// Module H: H1 imports E1, H2 is empty. Import H2 only — E2 is not visible.
-// =========================================================================
-//--- H/H1.h
-#include "E/E1.h"
-//--- H/H2.h
-// empty
-//--- H/module.modulemap
-module H {
-  module H1 { header "H1.h" }
-  module H2 { header "H2.h" }
-}
-
-// =========================================================================
-// Module J: another independent module that also includes Textual.h.
-// =========================================================================
-//--- J/J1.h
-#include "Textual.h"
-//--- J/module.modulemap
-module J {
-  module J1 { header "J1.h" }
-}
-
-// =========================================================================
-// Test 1: Cross-module, no export.
-// Import F1, which imports E1 (no export *). E2 included Textual.h but is
-// NOT visible. #import Textual.h must re-enter the file.
-// =========================================================================
-//--- test_cross_no_export.c
-#import "F/F1.h"
-#import "Textual.h"
-static int x = MACRO_TEXTUAL;
-
-// =========================================================================
-// Test 2: Cross-module, partial export chain.
-// Import G1 (export *) → E1 (no export *). E1 is visible but E2 is not.
-// Textual.h was included by E2 — must be re-enterable.
-// =========================================================================
-//--- test_partial_export.c
-#import "G/G1.h"
-#import "Textual.h"
-static int x = MACRO_TEXTUAL;
-
-// =========================================================================
-// Test 3: Import a sibling; the other sibling's transitive deps included
-// Textual.h. Import H2 only — H1 (and its dep E1, E2) are not visible.
-// Textual.h must be re-enterable.
-// =========================================================================
-//--- test_sibling_invisible.c
-#import "H/H2.h"
-#import "Textual.h"
-static int x = MACRO_TEXTUAL;
-
-// =========================================================================
-// Test 4: Multiple PCMs loaded, none with Textual.h visible.
-// Load both E.pcm and J.pcm. Import neither E2 nor J1.
-// Textual.h was included by both E2 and J1, but neither is visible.
-// Must be re-enterable.
-// =========================================================================
-//--- test_multiple_pcms.c
-// Don't import anything — just load PCMs via -fmodule-file.
-#import "Textual.h"
-static int x = MACRO_TEXTUAL;
-
-// =========================================================================
-// Test 5: Cross-module transitive, no export at any level.
-// Import F1 → E1 → E2 → Textual.h. Neither F1 nor E1 has export *.
-// E2 is not visible. Textual.h must be re-enterable.
-// =========================================================================
-//--- test_deep_no_export.c
-#import "F/F1.h"
-#import "Textual.h"
-static int x = MACRO_TEXTUAL;
-
-// =========================================================================
-// Build PCMs.
-// =========================================================================
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/E/module.modulemap -fmodule-name=E -o %t/E.pcm
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/F/module.modulemap -fmodule-name=F -fmodule-file=%t/E.pcm -o %t/F.pcm
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/G/module.modulemap -fmodule-name=G -fmodule-file=%t/E.pcm -o %t/G.pcm
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/H/module.modulemap -fmodule-name=H -fmodule-file=%t/E.pcm -o %t/H.pcm
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/J/module.modulemap -fmodule-name=J -o %t/J.pcm
-
-// =========================================================================
-// Run tests.
-// =========================================================================
-
-// Test 1: cross-module, no export — Textual.h must be re-entered.
-// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_cross_no_export.c -fmodule-file=%t/F.pcm
-
-// Test 2: partial export chain — Textual.h must be re-entered.
-// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_partial_export.c -fmodule-file=%t/G.pcm
-
-// Test 3: sibling invisible — Textual.h must be re-entered.
-// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_sibling_invisible.c -fmodule-file=%t/H.pcm
-
-// Test 4: multiple PCMs, none visible — Textual.h must be re-entered.
-// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_multiple_pcms.c -fmodule-file=%t/E.pcm -fmodule-file=%t/J.pcm
-
-// Test 5: deep transitive, no export — Textual.h must be re-entered.
-// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_deep_no_export.c -fmodule-file=%t/F.pcm
diff --git a/clang/test/Modules/import-textual-skip-multi-layer.c b/clang/test/Modules/import-textual-skip-multi-layer.c
deleted file mode 100644
index 888911e06f914..0000000000000
--- a/clang/test/Modules/import-textual-skip-multi-layer.c
+++ /dev/null
@@ -1,90 +0,0 @@
-// Test that #import does NOT re-enter textual headers included at multiple
-// layers of a cross-module dependency chain when the including modules ARE
-// visible (via transitive export *).
-//
-// Setup:
-//   Module A: A1 textually includes T1.h (export *)
-//   Module B: B1 imports A1 cross-module and textually includes T2.h (export *)
-//   Module C: C1 imports B1 cross-module (export *)
-//
-// The TU imports C1. Because every module in the chain has export *, B1 and
-// A1 are transitively visible. T1.h was included by A1 and T2.h was included
-// by B1 — both visible. So #import of T1.h and T2.h must be suppressed.
-
-// RUN: rm -rf %t
-// RUN: split-file %s %t
-
-//--- T1.h
-typedef int t1_type;
-#define MACRO_T1 10
-
-//--- T2.h
-typedef int t2_type;
-#define MACRO_T2 20
-
-// =========================================================================
-// Module A: A1 textually includes T1.h. Has export *.
-// =========================================================================
-//--- A/A1.h
-#include "T1.h"
-//--- A/module.modulemap
-module A {
-  module A1 {
-    header "A1.h"
-    export *
-  }
-}
-
-// =========================================================================
-// Module B: B1 imports A1 (cross-module) and textually includes T2.h.
-// Has export *.
-// =========================================================================
-//--- B/B1.h
-#include "A/A1.h"
-#include "T2.h"
-//--- B/module.modulemap
-module B {
-  module B1 {
-    header "B1.h"
-    export *
-  }
-}
-
-// =========================================================================
-// Module C: C1 imports B1 (cross-module). Has export *.
-// =========================================================================
-//--- C/C1.h
-#include "B/B1.h"
-//--- C/module.modulemap
-module C {
-  module C1 {
-    header "C1.h"
-    export *
-  }
-}
-
-// =========================================================================
-// Test: Import C1 (export *) → B1 (export *) → A1 (export *).
-// A1 and B1 are both visible. T1.h and T2.h were included by visible
-// modules, so #import must be suppressed. The typedefs in T1.h and T2.h
-// (no include guard) would cause duplicate definition errors if the files
-// were incorrectly re-entered.
-// =========================================================================
-//--- test.c
-#import "C/C1.h"
-#import "T1.h"
-#import "T2.h"
-t1_type x = MACRO_T1;
-t2_type y = MACRO_T2;
-
-// =========================================================================
-// Build PCMs.
-// =========================================================================
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/A/module.modulemap -fmodule-name=A -o %t/A.pcm
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/B/module.modulemap -fmodule-name=B -fmodule-file=%t/A.pcm -o %t/B.pcm
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/C/module.modulemap -fmodule-name=C -fmodule-file=%t/B.pcm -o %t/C.pcm
-
-// =========================================================================
-// Run test.
-// =========================================================================
-// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test.c -fmodule-file=%t/C.pcm
diff --git a/clang/test/Modules/import-textual-skip-visible.c b/clang/test/Modules/import-textual-skip-visible.c
deleted file mode 100644
index c107472128dbd..0000000000000
--- a/clang/test/Modules/import-textual-skip-visible.c
+++ /dev/null
@@ -1,186 +0,0 @@
-// Test that #import does NOT re-enter a textual header when the header was
-// included by a module that IS visible. The key invariant:
-//
-//   If a textual header was included by module M, and M is visible in the
-//   current TU (directly or via transitive export), then #import of that
-//   header MUST be skipped (the file is already included).
-//
-// This is the complement of import-textual-reenter-not-visible.c, which
-// verifies the opposite: non-visible modules do NOT suppress re-entry.
-
-// RUN: rm -rf %t
-// RUN: split-file %s %t
-
-// Textual.h has no include guard. If #import incorrectly re-enters the file,
-// the duplicate typedef will produce a compilation error.
-//--- Textual.h
-typedef int textual_type_t;
-#define MACRO_TEXTUAL 1
-
-// =========================================================================
-// Module E: E1 includes E2, E2 includes Textual.h. E1 has export *.
-// =========================================================================
-//--- E/E1.h
-#include "E2.h"
-//--- E/E2.h
-#include "Textual.h"
-//--- E/module.modulemap
-module E {
-  module E1 {
-    header "E1.h"
-    export *
-  }
-  module E2 { header "E2.h" }
-}
-
-// =========================================================================
-// Module F: F1 imports E1 cross-module. F1 has export *.
-// =========================================================================
-//--- F/F1.h
-#include "E/E1.h"
-//--- F/module.modulemap
-module F {
-  module F1 {
-    header "F1.h"
-    export *
-  }
-}
-
-// =========================================================================
-// Module G: G1 imports F1 cross-module. G1 has export *.
-// =========================================================================
-//--- G/G1.h
-#include "F/F1.h"
-//--- G/module.modulemap
-module G {
-  module G1 {
-    header "G1.h"
-    export *
-  }
-}
-
-// =========================================================================
-// Module A: A1 independently includes Textual.h (for early deserialization).
-// =========================================================================
-//--- A/A1.h
-#include "Textual.h"
-//--- A/module.modulemap
-module A {
-  module A1 { header "A1.h" }
-}
-
-// =========================================================================
-// Test 1: Sibling re-export within a single module.
-// Import E1 (export *) → E2 becomes visible → Textual.h's macro should
-// be available as a module macro without an explicit #import.
-// =========================================================================
-//--- test_sibling_reexport.c
-#import "E/E1.h"
-#ifdef MACRO_TEXTUAL
-textual_type_t val = MACRO_TEXTUAL;
-#else
-#error "MACRO_TEXTUAL should be visible via E1's re-export of E2"
-#endif
-
-// =========================================================================
-// Test 2: Cross-module transitive re-export (depth 2).
-// Import F1 (export *) → E1 (export *) → E2 visible. Macro should be
-// available without explicit #import.
-// =========================================================================
-//--- test_cross_depth2.c
-#import "F/F1.h"
-#ifdef MACRO_TEXTUAL
-textual_type_t val = MACRO_TEXTUAL;
-#else
-#error "MACRO_TEXTUAL should be visible via F1 -> E1 -> E2 export chain"
-#endif
-
-// =========================================================================
-// Test 3: Cross-module transitive re-export (depth 3).
-// Import G1 (export *) → F1 (export *) → E1 (export *) → E2 visible.
-// =========================================================================
-//--- test_cross_depth3.c
-#import "G/G1.h"
-#ifdef MACRO_TEXTUAL
-textual_type_t val = MACRO_TEXTUAL;
-#else
-#error "MACRO_TEXTUAL should be visible via G1 -> F1 -> E1 -> E2 export chain"
-#endif
-
-// =========================================================================
-// Test 4: Explicit #import of already-visible textual header is suppressed.
-// Import E1 (export *) → E2 visible → Textual.h already included.
-// A subsequent #import Textual.h should be a no-op. The typedef in
-// Textual.h (no include guard) would cause a duplicate definition error
-// if the file were incorrectly re-entered.
-// =========================================================================
-//--- test_import_suppressed.c
-#import "E/E1.h"
-#import "Textual.h"
-textual_type_t val = MACRO_TEXTUAL;
-
-// =========================================================================
-// Test 5: Cross-module #import suppression (depth 2).
-// Import F1 (export * chain) → E2 visible. Explicit #import of Textual.h
-// must be suppressed.
-// =========================================================================
-//--- test_import_suppressed_depth2.c
-#import "F/F1.h"
-#import "Textual.h"
-textual_type_t val = MACRO_TEXTUAL;
-
-// =========================================================================
-// Test 6: Cross-module #import suppression (depth 3).
-// Import G1 (export * chain) → E2 visible. Explicit #import of Textual.h
-// must be suppressed.
-// =========================================================================
-//--- test_import_suppressed_depth3.c
-#import "G/G1.h"
-#import "Textual.h"
-textual_type_t val = MACRO_TEXTUAL;
-
-// =========================================================================
-// Test 7: Early deserialization — import a different module that also
-// includes Textual.h BEFORE importing E1. This forces Textual.h's header
-// info to be deserialized (via markIncludedInModule) at a time when E2 is
-// NOT yet visible. Then import E1 (export *) → E2 becomes visible.
-// The subsequent #import Textual.h must still be suppressed.
-// =========================================================================
-//--- test_early_deser.c
-#import "A/A1.h"
-#import "E/E1.h"
-#import "Textual.h"
-textual_type_t val = MACRO_TEXTUAL;
-
-// =========================================================================
-// Build PCMs.
-// =========================================================================
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/E/module.modulemap -fmodule-name=E -o %t/E.pcm
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/F/module.modulemap -fmodule-name=F -fmodule-file=%t/E.pcm -o %t/F.pcm
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/G/module.modulemap -fmodule-name=G -fmodule-file=%t/F.pcm -o %t/G.pcm
-// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/A/module.modulemap -fmodule-name=A -o %t/A.pcm
-
-// =========================================================================
-// Run tests.
-// =========================================================================
-
-// Test 1: sibling re-export — macro visible without #import.
-// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_sibling_reexport.c -fmodule-file=%t/E.pcm
-
-// Test 2: depth-2 cross-module re-export — macro visible.
-// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_cross_depth2.c -fmodule-file=%t/F.pcm
-
-// Test 3: depth-3 cross-module re-export — macro visible.
-// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_cross_depth3.c -fmodule-file=%t/G.pcm
-
-// Test 4: explicit #import suppressed (single module).
-// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_import_suppressed.c -fmodule-file=%t/E.pcm
-
-// Test 5: explicit #import suppressed (depth 2).
-// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_import_suppressed_depth2.c -fmodule-file=%t/F.pcm
-
-// Test 6: explicit #import suppressed (depth 3).
-// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_import_suppressed_depth3.c -fmodule-file=%t/G.pcm
-
-// Test 7: early deserialization — #import suppressed despite prior deser.
-// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test_early_deser.c -fmodule-file=%t/A.pcm -fmodule-file=%t/E.pcm

>From 1141d8b6df6193e6e230548b54bcd026f521cb58 Mon Sep 17 00:00:00 2001
From: Qiongsi Wu <qiongsi_wu at apple.com>
Date: Mon, 27 Apr 2026 11:20:10 -0700
Subject: [PATCH 7/8] Adding documentation on non-modular headers.

---
 clang/docs/Modules.rst | 55 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/clang/docs/Modules.rst b/clang/docs/Modules.rst
index 379f62ebf548c..828017b59fd01 100644
--- a/clang/docs/Modules.rst
+++ b/clang/docs/Modules.rst
@@ -1081,6 +1081,61 @@ When writing a private module as part of a *framework*, it's recommended that:
   to work with this naming, using ``FooPrivate`` or ``Foo.Private`` (submodule)
   trigger warnings and might not work as expected.
 
+Non-modular Headers
+-------------------
+A non-modular header is a header file that is included or imported in modular
+headers, but is not a part of any modules (i.e., not listed in any module map).
+A non-modular header is considered visible in a translation unit only if it was
+included by a module that is itself visible.
+
+For example, consider the following setup. The inline comments describe how
+Clang behaves when importing a non-modular header.
+
+.. code-block:: c
+
+  // This example assumes the header search path is ./include.
+  /* include/NonModular.h */
+  #define MACRO_NON_MODULAR 1
+
+  /* include/A/module.modulemap */
+  module A {
+    header "A.h"
+  }
+
+  /* include/A/A.h */
+  // Module A includes a non-modular header.
+  #include "NonModular.h"
+
+  /* include/B/module.modulemap */
+  module B {
+    header "B.h"
+  }
+
+  /* include/B/B.h */
+  #include "A/A.h"
+
+  /* source_1.m */
+  #import "A/A.h"
+  // The import below is skipped, because the non-modular header is
+  // visible through module A. In other words, one can omit
+  // the import directive below, and the program compiles correctly.
+  #import "NonModular.h"
+
+  int foo() {
+    return MACRO_NON_MODULAR + 1;
+  }
+
+  /* source_2.m */
+  #import "B/B.h"
+  // Since B does not export its dependent modules, A is not visible.
+  // Hence "NonModular.h" is not visible. Thus clang re-enters "NonModular.h".
+  // The import directive below cannot be omitted.
+  #import "NonModular.h"
+
+  int foo() {
+    return MACRO_NON_MODULAR + 1;
+  }
+
 Modularizing a Platform
 =======================
 To get any benefit out of modules, one needs to introduce module maps for software libraries starting at the bottom of the stack. This typically means introducing a module map covering the operating system's headers and the C standard library headers (in ``/usr/include``, for a Unix system).

>From f4e047864900b1e7c23804fd21d989dd3d559648 Mon Sep 17 00:00:00 2001
From: Qiongsi Wu <qiongsi_wu at apple.com>
Date: Mon, 27 Apr 2026 15:49:25 -0700
Subject: [PATCH 8/8] Clean up the changes, adding an LSV test case, and adding
 documentations.

---
 clang/docs/Modules.rst                        |  2 +-
 clang/docs/ReleaseNotes.rst                   |  1 +
 clang/include/clang/Basic/Module.h            |  3 ++
 clang/include/clang/Lex/Preprocessor.h        |  6 ++++
 clang/lib/Lex/Preprocessor.cpp                |  3 +-
 clang/lib/Serialization/ASTReader.cpp         |  2 +-
 clang/lib/Serialization/ASTWriter.cpp         | 12 ++++----
 .../test/Modules/import-nonmodular-reenter.c  |  4 +--
 clang/test/Modules/import-nonmodular-skip.c   |  4 +--
 clang/test/Modules/per-submodule-includes.m   | 29 +++++++++++++------
 10 files changed, 45 insertions(+), 21 deletions(-)

diff --git a/clang/docs/Modules.rst b/clang/docs/Modules.rst
index 828017b59fd01..130a67248e7ef 100644
--- a/clang/docs/Modules.rst
+++ b/clang/docs/Modules.rst
@@ -1132,7 +1132,7 @@ Clang behaves when importing a non-modular header.
   // The import directive below cannot be omitted.
   #import "NonModular.h"
 
-  int foo() {
+  int bar() {
     return MACRO_NON_MODULAR + 1;
   }
 
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 03362cf4e0f8a..ea700b1de77db 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -497,6 +497,7 @@ Bug Fixes in This Version
 - Fixed a crash where constexpr evaluation encountered invalid overrides. (#GH183290)
 - Fixed a crash when assigning to an element of an ``ext_vector_type`` with ``bool`` element type. (#GH189260)
 - Clang now emits an error for friend declarations of lambda members. (#GH26540)
+- Fixed ``#import`` incorrectly skipping non-modular headers when the header was included by a module that is not visible in the current translation unit. Non-modular headers are now only considered "already imported" when the including module is visible. (#GH170215)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Basic/Module.h b/clang/include/clang/Basic/Module.h
index 5cf39a8c5e2e1..0fd78815ac713 100644
--- a/clang/include/clang/Basic/Module.h
+++ b/clang/include/clang/Basic/Module.h
@@ -595,6 +595,9 @@ class alignas(8) Module {
   /// to import but didn't because they are not direct uses.
   llvm::SmallSetVector<const Module *, 2> UndeclaredUses;
 
+  /// The set of files included during the build of this module/submodule.
+  /// Used to track per-submodule header inclusion for visibility-aware
+  /// #import semantics.
   llvm::DenseSet<const FileEntry *> Includes;
 
   /// A library or framework to link against when an entity from this
diff --git a/clang/include/clang/Lex/Preprocessor.h b/clang/include/clang/Lex/Preprocessor.h
index c067dec113e60..87759f2d2016d 100644
--- a/clang/include/clang/Lex/Preprocessor.h
+++ b/clang/include/clang/Lex/Preprocessor.h
@@ -1574,11 +1574,17 @@ class Preprocessor {
     return CurSubmoduleState->VisibleIncludedFiles.contains(File);
   }
 
+  /// Mark a file as included at the top level (outside any module).
+  /// Called by ASTReader when deserializing header file info.
   void markIncludedOnTopLevel(const FileEntry *File) {
     Includes.insert(File);
     CurSubmoduleState->VisibleIncludedFiles.insert(File);
   }
 
+  /// Mark a file as included by module \p M.
+  /// Called by ASTReader when deserializing header file info.
+  /// The file is added to VisibleIncludedFiles only if \p M is currently
+  /// visible.
   void markIncludedInModule(Module *M, const FileEntry *File) {
     M->Includes.insert(File);
 
diff --git a/clang/lib/Lex/Preprocessor.cpp b/clang/lib/Lex/Preprocessor.cpp
index ae159956f331d..b08459632aacb 100644
--- a/clang/lib/Lex/Preprocessor.cpp
+++ b/clang/lib/Lex/Preprocessor.cpp
@@ -1443,7 +1443,8 @@ void Preprocessor::makeModuleVisible(Module *M, SourceLocation Loc,
         // FIXME: Include the path in the diagnostic.
         // FIXME: Include the import location for the conflicting module.
         Diag(ModuleImportLoc, diag::warn_module_conflict)
-            << Path[0]->getFullModuleName() << Conflict->getFullModuleName()
+            << Path[0]->getFullModuleName()
+            << Conflict->getFullModuleName()
             << Message;
       });
 
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index c17f5c89d381a..124fdb71d1cc6 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -2394,7 +2394,6 @@ HeaderFileInfoTrait::ReadData(internal_key_ref key, const unsigned char *d,
 
   auto FE = getFile(key);
   Preprocessor &PP = Reader.getPreprocessor();
-  ModuleMap &ModMap = PP.getHeaderSearchInfo().getModuleMap();
 
   unsigned IncludedCount =
       endian::readNext<uint32_t, llvm::endianness::little, unaligned>(d);
@@ -2420,6 +2419,7 @@ HeaderFileInfoTrait::ReadData(internal_key_ref key, const unsigned char *d,
 
   assert((End - d) % 4 == 0 &&
          "Wrong data length in HeaderFileInfo deserialization");
+  ModuleMap &ModMap = PP.getHeaderSearchInfo().getModuleMap();
   while (d != End) {
     uint32_t LocalSMID =
         endian::readNext<uint32_t, llvm::endianness::little>(d);
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index d240d5d19914a..78a4544b20a83 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -2296,15 +2296,17 @@ void ASTWriter::WriteHeaderSearch(const HeaderSearch &HS) {
     std::vector<const Module *> Includers;
     if (WritingModule) {
       llvm::DenseSet<const Module *> Seen;
-      std::function<void(const Module *)> Visit = [&](const Module *M) {
+      SmallVector<const Module *, 16> Worklist;
+      Worklist.push_back(WritingModule);
+      while (!Worklist.empty()) {
+        const Module *M = Worklist.pop_back_val();
         if (!Seen.insert(M).second)
-          return;
+          continue;
         if (M->Includes.contains(*File))
           Includers.push_back(M);
         for (const Module *SubM : M->submodules())
-          Visit(SubM);
-      };
-      Visit(WritingModule);
+          Worklist.push_back(SubM);
+      }
     } else if (PP->getTopLevelIncludes().contains(*File)) {
       Includers.push_back(nullptr);
     }
diff --git a/clang/test/Modules/import-nonmodular-reenter.c b/clang/test/Modules/import-nonmodular-reenter.c
index 16a43133f7dea..34d3e6b0e453f 100644
--- a/clang/test/Modules/import-nonmodular-reenter.c
+++ b/clang/test/Modules/import-nonmodular-reenter.c
@@ -59,7 +59,7 @@ static int x = MACRO_NON_MODULAR;
 
 // Build the pcms
 // RUN: %clang_cc1 -fmodules -I %t/include -emit-module %t/include/A/module.modulemap -fmodule-name=A -o %t/A.pcm
-// RUN: %clang_cc1 -fmodules -I %t/include -emit-module %t/include/B/module.modulemap -fmodule-name=B -o %t/B.pcm
+// RUN: %clang_cc1 -fmodules -I %t/include -emit-module %t/include/B/module.modulemap -fmodule-name=B -fmodule-file=%t/A.pcm -o %t/B.pcm
 
 // Test case 1: loading the pcm but not importing it in the TU.
 // RUN: %clang_cc1 -fmodules -I %t/include -fsyntax-only %t/test_pcm_loaded.c -fmodule-file=%t/A.pcm
@@ -67,5 +67,5 @@ static int x = MACRO_NON_MODULAR;
 // Test case 2: invisible sibling.
 // RUN: %clang_cc1 -fmodules -I %t/include -fsyntax-only %t/test_invisible_sibling.c -fmodule-file=%t/A.pcm
 
-// Test case 3: invisible transtivie imported module.
+// Test case 3: invisible transitive imported module.
 // RUN: %clang_cc1 -fmodules -I %t/include -fsyntax-only %t/test_invisible_transitive.c -fmodule-file=%t/B.pcm
diff --git a/clang/test/Modules/import-nonmodular-skip.c b/clang/test/Modules/import-nonmodular-skip.c
index e9711a7fb353a..f4fe8cf4e6103 100644
--- a/clang/test/Modules/import-nonmodular-skip.c
+++ b/clang/test/Modules/import-nonmodular-skip.c
@@ -3,7 +3,7 @@
 //
 //   If a non-modular header was included by module M, and M is visible in the
 //   current TU, then #import of that header skips importing the header. In
-//   other words, the #import sematic (import only once) is satisfied.
+//   other words, the #import semantic (import only once) is satisfied.
 //
 // We cover two different cases, and we skip the header in the
 // translation unit for both of them:
@@ -54,7 +54,7 @@ non_modular_type_t val = MACRO_NON_MODULAR;
 
 // Build the pcms
 // RUN: %clang_cc1 -fmodules -I %t/include -emit-module %t/include/A/module.modulemap -fmodule-name=A -o %t/A.pcm
-// RUN: %clang_cc1 -fmodules -I %t/include -emit-module %t/include/B/module.modulemap -fmodule-name=B -o %t/B.pcm
+// RUN: %clang_cc1 -fmodules -I %t/include -emit-module %t/include/B/module.modulemap -fmodule-name=B -fmodule-file=%t/A.pcm -o %t/B.pcm
 
 // Test 1: directly importing a submodule that includes a non-modular header.
 // RUN: %clang_cc1 -fmodules -I %t/include -fsyntax-only %t/test_direct_import.c -fmodule-file=%t/A.pcm
diff --git a/clang/test/Modules/per-submodule-includes.m b/clang/test/Modules/per-submodule-includes.m
index 39fd255e8e93e..77c6be9ad1674 100644
--- a/clang/test/Modules/per-submodule-includes.m
+++ b/clang/test/Modules/per-submodule-includes.m
@@ -29,25 +29,36 @@
 #import <Textual/Header.h>
 int fn() { return symbol; }
 
-// Compilation using the PCH regularly succeeds. The import of FW/Sub1.h in the
-// PCH is treated textually due to -fmodule-name=FW.
+// The import of FW/Sub1.h in the PCH is treated textually due to -fmodule-name=FW.
+// Because of this, pch.h.pch only imports an empty file (FW/Sub1.h), not
+// the module FW. Therefore importing Textual/Header.h just works.
 //
 // RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fimplicit-module-maps -F %t/frameworks -fmodule-name=FW \
-// RUN:   -emit-pch -x objective-c %t/pch.h -o %t/pch.h.gch
+// RUN:   -emit-pch -x objective-c %t/pch.h -o %t/pch.h.pch
 //
 // RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fimplicit-module-maps -F %t/frameworks -fmodule-name=FW \
-// RUN:   -include-pch %t/pch.h.gch -fsyntax-only %t/tu.m
+// RUN:   -include-pch %t/pch.h.pch -fsyntax-only %t/tu.m
 
-// Compilation using the PCH as precompiled module fails. The import of FW/Sub1.h
-// in the PCH is translated to an import. Nothing is preventing that now that
-// -fmodule-name=FW has been replaced with -fmodule-name=__PCH.
+// Building the __PCH module treats #import <FW/Sub1.h> as a module import from
+// pch.h. Such a module import triggers a build of the module FW. Since FW/Sub2.h
+// imports Textual/Header.h, Textual/Header.h becomes visible in submodule Sub2.
 //
 // RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fimplicit-module-maps -F %t/frameworks \
 // RUN:   -emit-module -fmodule-name=__PCH -x objective-c %t/pch.modulemap -o %t/pch.h.pcm
 //
 // Loading FW.pcm marks Textual/Header.h as imported (because it is imported in
-// FW.Sub2), so the TU does not import it again. It's contents remain invisible,
-// though.
+// FW.Sub2), so the TU does not import it again.
 //
 // RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fimplicit-module-maps -F %t/frameworks \
 // RUN:   -include %t/pch.h -fmodule-map-file=%t/pch.modulemap -fsyntax-only %t/tu.m
+
+// With local submodule visibility, importing FW.Sub1 does NOT make FW.Sub2
+// visible. Textual/Header.h was included by Sub2, which is not visible, so
+// #import re-enters the header textually making its contents available.
+//
+// RUN: rm -rf %t/cache
+// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility -fmodules-cache-path=%t/cache -fimplicit-module-maps -F %t/frameworks \
+// RUN:   -emit-module -fmodule-name=__PCH -x objective-c %t/pch.modulemap -o %t/pch.lsv.pcm
+//
+// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility -fmodules-cache-path=%t/cache -fimplicit-module-maps -F %t/frameworks \
+// RUN:   -include %t/pch.h -fmodule-map-file=%t/pch.modulemap -fsyntax-only %t/tu.m