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

[Qiongsi Wu via cfe-commits]

================
@@ -1550,19 +1553,46 @@ 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->VisibleIncludedFiles.insert(File);
+
+    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);
-    return IncludedFiles.count(File);
+    return CurSubmoduleState->VisibleIncludedFiles.contains(File);
----------------
qiongsiwu wrote:

Thanks for the test case! I fixed this differently by adding a mapping from header files to sets of modules that include them. The map lives in the preprocessor. For a particular header, we can now loop through the including modules to see if any of them is visible. I think if we do not have a textual header included in many modules simultaneously, this should be cheaper than looping through all modules coming from header search (e.g.  https://github.com/llvm/llvm-project/pull/71117/changes#diff-d80edc2f8b14e8ad634b54bc14c780e34a3d7c716b8b9811d1f831135b470f5eR1505-R1509). The header search loop also seem to have a different problem. `HeaderInfo.getModuleMap().modules()` seem to only return top level modules? So `lsv` is broken if I use the loop verbatim.  Test `clang/test/Modules/import-nonmodular-lsv-sibling-deserialization.c` shows that. With the `IncludingModules` mapping, we can handle submodules. 

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