[clang][modules] Avoid re-exporting PCH imports on every later module import

We only want to make PCH imports visible once for the the TU, not
repeatedly after every subsequent import. This causes some incorrect
behaviour with submodule visibility, and causes us to get extra module
dependencies in the scanner. So far I have only seen obviously incorrect
behaviour when building with -fmodule-name to cause a submodule to be
textually included when using the PCH, though the old behaviour seems
wrong regardless.

rdar://107449644

Differential Revision: https://reviews.llvm.org/D148176

clang/include/clang/Serialization/ASTReader.h

@@ -948,8 +948,14 @@ public:
 
 private:
   /// A list of modules that were imported by precompiled headers or
-  /// any other non-module AST file.
-  SmallVector<ImportedSubmodule, 2> ImportedModules;
+  /// any other non-module AST file and have not yet been made visible. If a
+  /// module is made visible in the ASTReader, it will be transfered to
+  /// \c PendingImportedModulesSema.
+  SmallVector<ImportedSubmodule, 2> PendingImportedModules;
+
+  /// A list of modules that were imported by precompiled headers or
+  /// any other non-module AST file and have not yet been made visible for Sema.
+  SmallVector<ImportedSubmodule, 2> PendingImportedModulesSema;
   //@}
 
   /// The system include root to be used when loading the

clang/lib/Serialization/ASTReader.cpp

@@ -3728,7 +3728,7 @@ llvm::Error ASTReader::ReadASTBlock(ModuleFile &F,
           unsigned GlobalID = getGlobalSubmoduleID(F, Record[I++]);
           SourceLocation Loc = ReadSourceLocation(F, Record, I);
           if (GlobalID) {
-            ImportedModules.push_back(ImportedSubmodule(GlobalID, Loc));
+            PendingImportedModules.push_back(ImportedSubmodule(GlobalID, Loc));
             if (DeserializationListener)
               DeserializationListener->ModuleImportRead(GlobalID, Loc);
           }
@@ -4445,8 +4445,8 @@ ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName,
   UnresolvedModuleRefs.clear();
 
   if (Imported)
-    Imported->append(ImportedModules.begin(),
-                     ImportedModules.end());
+    Imported->append(PendingImportedModules.begin(),
+                     PendingImportedModules.end());
 
   // FIXME: How do we load the 'use'd modules? They may not be submodules.
   // Might be unnecessary as use declarations are only used to build the
@@ -5050,7 +5050,7 @@ void ASTReader::InitializeContext() {
 
   // Re-export any modules that were imported by a non-module AST file.
   // FIXME: This does not make macro-only imports visible again.
-  for (auto &Import : ImportedModules) {
+  for (auto &Import : PendingImportedModules) {
     if (Module *Imported = getSubmodule(Import.ID)) {
       makeModuleVisible(Imported, Module::AllVisible,
                         /*ImportLoc=*/Import.ImportLoc);
@@ -5060,6 +5060,10 @@ void ASTReader::InitializeContext() {
       // nullptr here, we do the same later, in UpdateSema().
     }
   }
+
+  // Hand off these modules to Sema.
+  PendingImportedModulesSema.append(PendingImportedModules);
+  PendingImportedModules.clear();
 }
 
 void ASTReader::finalizeForWriting() {
@@ -8105,13 +8109,14 @@ void ASTReader::UpdateSema() {
   }
 
   // For non-modular AST files, restore visiblity of modules.
-  for (auto &Import : ImportedModules) {
+  for (auto &Import : PendingImportedModulesSema) {
     if (Import.ImportLoc.isInvalid())
       continue;
     if (Module *Imported = getSubmodule(Import.ID)) {
       SemaObj->makeModuleVisible(Imported, Import.ImportLoc);
     }
   }
+  PendingImportedModulesSema.clear();
 }
 
 IdentifierInfo *ASTReader::get(StringRef Name) {

clang/test/ClangScanDeps/modules-pch-imports.c

@@ -0,0 +1,108 @@
+// Check that a module from -fmodule-name= does not accidentally pick up extra
+// dependencies that come from a PCH.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+// RUN: sed "s|DIR|%/t|g" %t/cdb_pch.json.template > %t/cdb_pch.json
+
+// Scan PCH
+// RUN: clang-scan-deps -compilation-database %t/cdb_pch.json \
+// RUN:   -format experimental-full -mode preprocess-dependency-directives \
+// RUN:   > %t/deps_pch.json
+
+// Build PCH
+// RUN: %deps-to-rsp %t/deps_pch.json --module-name A > %t/A.rsp
+// RUN: %deps-to-rsp %t/deps_pch.json --module-name B > %t/B.rsp
+// RUN: %deps-to-rsp %t/deps_pch.json --tu-index 0 > %t/pch.rsp
+// RUN: %clang @%t/A.rsp
+// RUN: %clang @%t/B.rsp
+// RUN: %clang @%t/pch.rsp
+
+// Scan TU with PCH
+// RUN: clang-scan-deps -compilation-database %t/cdb.json \
+// RUN:   -format experimental-full -mode preprocess-dependency-directives \
+// RUN:   > %t/deps.json
+
+// RUN: cat %t/deps.json | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t
+
+// Verify that the only modular import in C is E and not the unrelated modules
+// A or B that come from the PCH.
+
+// CHECK:      {
+// CHECK-NEXT:  "modules": [
+// CHECK-NEXT:     {
+// CHECK:            "clang-module-deps": [
+// CHECK-NEXT:         {
+// CHECK:                "module-name": "E"
+// CHECK:              }
+// CHECK-NEXT:       ]
+// CHECK:            "clang-modulemap-file": "[[PREFIX]]/module.modulemap"
+// CHECK:            "command-line": [
+// CHECK-NOT:          "-fmodule-file=
+// CHECK:              "-fmodule-file={{(E=)?}}[[PREFIX]]/{{.*}}/E-{{.*}}.pcm"
+// CHECK-NOT:          "-fmodule-file=
+// CHECK:            ]
+// CHECK:            "name": "C"
+// CHECK:          }
+
+
+//--- cdb_pch.json.template
+[{
+  "file": "DIR/prefix.h",
+  "directory": "DIR",
+  "command": "clang -x c-header DIR/prefix.h -o DIR/prefix.h.pch -fmodules -fimplicit-modules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache"
+}]
+
+//--- cdb.json.template
+[{
+  "file": "DIR/tu.c",
+  "directory": "DIR",
+  "command": "clang -fsyntax-only DIR/tu.c -include DIR/prefix.h -fmodule-name=C -fmodules -fimplicit-modules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache"
+}]
+
+//--- module.modulemap
+module A { header "A.h" export * }
+module B { header "B.h" export * }
+module C { header "C.h" export * }
+module D { header "D.h" export * }
+module E { header "E.h" export * }
+
+//--- A.h
+#pragma once
+struct A { int x; };
+
+//--- B.h
+#pragma once
+#include "A.h"
+struct B { struct A a; };
+
+//--- C.h
+#pragma once
+#include "E.h"
+struct C { struct E e; };
+
+//--- D.h
+#pragma once
+#include "C.h"
+struct D { struct C c; };
+
+//--- E.h
+#pragma once
+struct E { int y; };
+
+//--- prefix.h
+#include "B.h"
+
+//--- tu.c
+// C.h is first included textually due to -fmodule-name=C.
+#include "C.h"
+// importing D pulls in a modular import of C; it's this build of C that we
+// are verifying above
+#include "D.h"
+
+void tu(void) {
+  struct A a;
+  struct B b;
+  struct C c;
+}

clang/test/Modules/submodule-visibility-pch.c

@@ -0,0 +1,56 @@
+// Verify that the use of a PCH does not accidentally make modules from the PCH
+// visible to submodules when using -fmodules-local-submodule-visibility
+// and -fmodule-name to trigger a textual include.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// First check that it works with a header
+
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache \
+// RUN:   -fmodules-local-submodule-visibility -fimplicit-module-maps \
+// RUN:   -fmodule-name=Mod \
+// RUN:   %t/tu.c -include %t/prefix.h -I %t -verify
+
+// Now with a PCH
+
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache \
+// RUN:   -fmodules-local-submodule-visibility -fimplicit-module-maps \
+// RUN:   -x c-header %t/prefix.h -emit-pch -o %t/prefix.pch -I %t
+
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache \
+// RUN:   -fmodules-local-submodule-visibility -fimplicit-module-maps \
+// RUN:   -fmodule-name=Mod \
+// RUN:   %t/tu.c -include-pch %t/prefix.pch -I %t -verify
+
+//--- module.modulemap
+module ModViaPCH { header "ModViaPCH.h" }
+module ModViaInclude { header "ModViaInclude.h" }
+module Mod { header "Mod.h" }
+module SomeOtherMod { header "SomeOtherMod.h" }
+
+//--- ModViaPCH.h
+#define ModViaPCH 1
+
+//--- ModViaInclude.h
+#define ModViaInclude 2
+
+//--- SomeOtherMod.h
+// empty
+
+//--- Mod.h
+#include "SomeOtherMod.h"
+#ifdef ModViaPCH
+#error "Visibility violation ModViaPCH"
+#endif
+#ifdef ModViaInclude
+#error "Visibility violation ModViaInclude"
+#endif
+
+//--- prefix.h
+#include "ModViaPCH.h"
+
+//--- tu.c
+#include "ModViaInclude.h"
+#include "Mod.h"
+// expected-no-diagnostics