From e06a91c5996b039cacd55e6ead0baf14424c740c Mon Sep 17 00:00:00 2001 From: Ben Langmuir Date: Wed, 12 Apr 2023 14:54:55 -0700 Subject: [PATCH] [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 | 10 +- clang/lib/Serialization/ASTReader.cpp | 15 ++- .../test/ClangScanDeps/modules-pch-imports.c | 108 ++++++++++++++++++ clang/test/Modules/submodule-visibility-pch.c | 56 +++++++++ 4 files changed, 182 insertions(+), 7 deletions(-) create mode 100644 clang/test/ClangScanDeps/modules-pch-imports.c create mode 100644 clang/test/Modules/submodule-visibility-pch.c diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h index 4f9457d8653e..1360ee1877c1 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/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 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 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 PendingImportedModulesSema; //@} /// The system include root to be used when loading the diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index 9dbe56123e40..b27304deb33c 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/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) { diff --git a/clang/test/ClangScanDeps/modules-pch-imports.c b/clang/test/ClangScanDeps/modules-pch-imports.c new file mode 100644 index 000000000000..63c6055efe06 --- /dev/null +++ b/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; +} diff --git a/clang/test/Modules/submodule-visibility-pch.c b/clang/test/Modules/submodule-visibility-pch.c new file mode 100644 index 000000000000..648f756fb971 --- /dev/null +++ b/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