Author: Ben Langmuir Date: 2023-04-20T11:29:23-07:00 New Revision: e06a91c5996b039cacd55e6ead0baf14424c740c
URL: https://github.com/llvm/llvm-project/commit/e06a91c5996b039cacd55e6ead0baf14424c740c DIFF: https://github.com/llvm/llvm-project/commit/e06a91c5996b039cacd55e6ead0baf14424c740c.diff LOG: [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 Added: clang/test/ClangScanDeps/modules-pch-imports.c clang/test/Modules/submodule-visibility-pch.c Modified: clang/include/clang/Serialization/ASTReader.h clang/lib/Serialization/ASTReader.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h index 4f9457d8653e3..1360ee1877c1a 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -948,8 +948,14 @@ class ASTReader 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 diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index 9dbe56123e40e..b27304deb33c9 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 0000000000000..63c6055efe061 --- /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 0000000000000..648f756fb9713 --- /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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
