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

[benlangmuir]

Summary

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

[benlangmuir]

Ping

[jansvoboda11]

Can you explain why we need to keep separate PendingImportedModulesSema now? In what situation will it end up aggregating more than one PendingImportedModules?

[benlangmuir]

In D148176#4283942, @jansvoboda11 wrote:

Can you explain why we need to keep separate PendingImportedModulesSema now? In what situation will it end up aggregating more than one PendingImportedModules?

I don't know if this happens in practice, but I haven't been able to prove to myself that it cannot. I'm not sure there is much simplification we can get here regardless, because we still need some kind of state tracking here for the case when Sema is not provided until later. If we are sure it can never add more PendingImportedModules later, then a flag telling us whether we have handled these modules in PP (but not yet Sema) would be sufficient.

[jansvoboda11]

That makes sense to me, thanks!

clang/include/clang/Serialization/ASTReader.h

struct ImportedSubmodule {
     SourceLocation ImportLoc;
 
     ImportedSubmodule(serialization::SubmoduleID ID, SourceLocation ImportLoc)
         : ID(ID), ImportLoc(ImportLoc) {}
   };
 
 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
   /// precompiled header.
   std::string isysroot;
 
   /// Whether to disable the normal validation performed on precompiled
   /// headers and module files when they are loaded.

clang/lib/Serialization/ASTReader.cpp

case IMPORTED_MODULES:
       if (!F.isModule()) {
         // If we aren't loading a module (which has its own exports), make
         // all of the imported modules visible.
         // FIXME: Deal with macros-only imports.
         for (unsigned I = 0, N = Record.size(); I != N; /**/) {
           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);
           }
         }
       }
       break;
 
     case MACRO_OFFSET: {
case UnresolvedModuleRef::Export:
         Unresolved.Mod->Exports.push_back(
           Module::ExportDecl(ResolvedMod, Unresolved.IsWildcard));
       continue;
     }
   }
   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
   // module itself.
 
   if (ContextObj)
     InitializeContext();
 
void ASTReader::InitializeContext() {
   if (!CUDASpecialDeclRefs.empty()) {
     assert(CUDASpecialDeclRefs.size() == 1 && "More decl refs than expected!");
     Context.setcudaConfigureCallDecl(
                            cast<FunctionDecl>(GetDecl(CUDASpecialDeclRefs[0])));
   }
 
   // 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);
       if (Import.ImportLoc.isValid())
         PP.makeModuleVisible(Imported, Import.ImportLoc);
       // This updates visibility for Preprocessor only. For Sema, which can be
       // nullptr here, we do the same later, in UpdateSema().
     }
   }
+
+  // Hand off these modules to Sema.
+  PendingImportedModulesSema.append(PendingImportedModules);
+  PendingImportedModules.clear();
 }
 
 void ASTReader::finalizeForWriting() {
   // Nothing to do for now.
 }
 
 /// Reads and return the signature record from \p PCH's control block, or
 /// else returns 0.
if (FpPragmaCurrentLocation.isInvalid()) {
       // Keep the current values.
     } else {
       SemaObj->FpPragmaStack.CurrentValue = *FpPragmaCurrentValue;
       SemaObj->FpPragmaStack.CurrentPragmaLocation = FpPragmaCurrentLocation;
     }
   }
 
   // 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) {
   // Note that we are loading an identifier.
   Deserializing AnIdentifier(this);
 
   IdentifierLookupVisitor Visitor(Name, /*PriorGeneration=*/0,
                                   NumIdentifierLookups,

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

+// 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

+// 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