[clang] [clang][DependencyScanning] Track dependencies from prebuilt modules to determine IsInStableDir (PR #132237)
Cyndy Ishida via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 8 10:57:14 PDT 2025
https://github.com/cyndyishida updated https://github.com/llvm/llvm-project/pull/132237
>From 3aaaf3371215de0f214836da32f862518f223760 Mon Sep 17 00:00:00 2001
From: Cyndy Ishida <cyndy_ishida at apple.com>
Date: Wed, 12 Mar 2025 21:26:36 -0700
Subject: [PATCH 1/3] [clang][DependencyScanning] Track dependencies from
prebuilt modules to determine IsInStableDir
When a module is being scanned, it can depend on modules that have
already been built from a pch dependency. When this happens, the
pcm files are reused for the module dependencies. When this is the case,
check if input files recorded from the PCMs come from the provided stable directories transitively,
since the scanner will not have access to the full set of file dependencies from prebuilt modules.
---
clang/include/clang/Serialization/ASTReader.h | 11 ++
.../DependencyScanning/ModuleDepCollector.h | 59 ++++++++-
clang/lib/Frontend/FrontendActions.cpp | 7 +-
clang/lib/Serialization/ASTReader.cpp | 6 +-
.../DependencyScanningWorker.cpp | 112 +++++++++++++++---
.../DependencyScanning/ModuleDepCollector.cpp | 89 ++++++++------
.../prebuilt-modules-in-stable-dirs.c | 24 +++-
7 files changed, 237 insertions(+), 71 deletions(-)
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index 57ae4aa104d9a..617ac23984b60 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -237,6 +237,17 @@ class ASTReaderListener {
return true;
}
+ /// Overloaded member function of \c visitInputFile that should
+ /// be defined when the input file contains both the virtual and external
+ /// paths, for example when deserializing input files from AST files.
+ ///
+ /// \returns true to continue receiving the next input file, false to stop.
+ virtual bool visitInputFile(StringRef FilenameAsRequested, StringRef Filename,
+ bool isSystem, bool isOverridden,
+ bool isExplicitModule) {
+ return true;
+ }
+
/// Returns true if this \c ASTReaderListener wants to receive the
/// imports of the AST file via \c visitImport, false otherwise.
virtual bool needsImportVisitation() const { return false; }
diff --git a/clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h b/clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
index ed150b467e3a1..ce5e67d2624d9 100644
--- a/clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
+++ b/clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
@@ -33,6 +33,7 @@ namespace dependencies {
class DependencyActionController;
class DependencyConsumer;
+class PrebuiltModuleASTAttrs;
/// Modular dependency that has already been built prior to the dependency scan.
struct PrebuiltModuleDep {
@@ -46,6 +47,47 @@ struct PrebuiltModuleDep {
ModuleMapFile(M->PresumedModuleMapFile) {}
};
+/// Attributes loaded from AST files of prebuilt modules collected prior to
+/// ModuleDepCollector creation.
+using PrebuiltModulesAttrsMap = llvm::StringMap<PrebuiltModuleASTAttrs>;
+class PrebuiltModuleASTAttrs {
+public:
+ /// When a module is discovered to not be in stable directories, traverse &
+ /// update all modules that depend on it.
+ void
+ updateDependentsNotInStableDirs(PrebuiltModulesAttrsMap &PrebuiltModulesMap);
+
+ /// Read-only access to whether the module is made up of dependencies in
+ /// stable directories.
+ bool isInStableDir() const { return IsInStableDirs; }
+
+ /// Read-only access to vfs map files.
+ const llvm::StringSet<> &getVFS() const { return VFSMap; }
+
+ /// Update the VFSMap to the one discovered from serializing the AST file.
+ void setVFS(llvm::StringSet<> &&VFS) { VFSMap = std::move(VFS); }
+
+ /// Add a direct dependent module file, so it can be updated if the current
+ /// module is from stable directores.
+ void addDependent(StringRef ModuleFile) {
+ ModuleFileDependents.insert(ModuleFile);
+ }
+
+ /// Update whether the prebuilt module resolves entirely in a stable
+ /// directories.
+ void setInStableDir(bool V = false) {
+ // Cannot reset attribute once it's false.
+ if (!IsInStableDirs)
+ return;
+ IsInStableDirs = V;
+ }
+
+private:
+ llvm::StringSet<> VFSMap;
+ bool IsInStableDirs = true;
+ std::set<StringRef> ModuleFileDependents;
+};
+
/// This is used to identify a specific module.
struct ModuleID {
/// The name of the module. This may include `:` for C++20 module partitions,
@@ -171,8 +213,6 @@ struct ModuleDeps {
BuildInfo;
};
-using PrebuiltModuleVFSMapT = llvm::StringMap<llvm::StringSet<>>;
-
class ModuleDepCollector;
/// Callback that records textual includes and direct modular includes/imports
@@ -242,7 +282,7 @@ class ModuleDepCollector final : public DependencyCollector {
CompilerInstance &ScanInstance, DependencyConsumer &C,
DependencyActionController &Controller,
CompilerInvocation OriginalCI,
- PrebuiltModuleVFSMapT PrebuiltModuleVFSMap);
+ const PrebuiltModulesAttrsMap PrebuiltModulesASTMap);
void attachToPreprocessor(Preprocessor &PP) override;
void attachToASTReader(ASTReader &R) override;
@@ -262,8 +302,9 @@ class ModuleDepCollector final : public DependencyCollector {
DependencyConsumer &Consumer;
/// Callbacks for computing dependency information.
DependencyActionController &Controller;
- /// Mapping from prebuilt AST files to their sorted list of VFS overlay files.
- PrebuiltModuleVFSMapT PrebuiltModuleVFSMap;
+ /// Mapping from prebuilt AST filepaths to their attributes referenced during
+ /// dependency collecting.
+ const PrebuiltModulesAttrsMap PrebuiltModulesASTMap;
/// Path to the main source file.
std::string MainFile;
/// Hash identifying the compilation conditions of the current TU.
@@ -339,6 +380,14 @@ void resetBenignCodeGenOptions(frontend::ActionKind ProgramAction,
bool isPathInStableDir(const ArrayRef<StringRef> Directories,
const StringRef Input);
+/// Determine if options collected from a module's
+/// compilation can safely be considered as stable.
+///
+/// \param Directories Paths known to be in a stable location. e.g. Sysroot.
+/// \param HSOpts Header search options derived from the compiler invocation.
+bool areOptionsInStableDir(const ArrayRef<StringRef> Directories,
+ const HeaderSearchOptions &HSOpts);
+
} // end namespace dependencies
} // end namespace tooling
} // end namespace clang
diff --git a/clang/lib/Frontend/FrontendActions.cpp b/clang/lib/Frontend/FrontendActions.cpp
index bf273c82a96e8..c5aeb92c7af73 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -777,10 +777,11 @@ namespace {
/// Indicates that the AST file contains particular input file.
///
/// \returns true to continue receiving the next input file, false to stop.
- bool visitInputFile(StringRef Filename, bool isSystem,
- bool isOverridden, bool isExplicitModule) override {
+ bool visitInputFile(StringRef FilenameAsRequested, StringRef Filename,
+ bool isSystem, bool isOverridden,
+ bool isExplicitModule) override {
- Out.indent(2) << "Input file: " << Filename;
+ Out.indent(2) << "Input file: " << FilenameAsRequested;
if (isSystem || isOverridden || isExplicitModule) {
Out << " [";
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index d8d77e7f55232..9b78071ade4cd 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -5835,9 +5835,13 @@ bool ASTReader::readASTFileControlBlock(
break;
case INPUT_FILE:
bool Overridden = static_cast<bool>(Record[3]);
+ size_t FilenameLen = ModuleDir.size() + Record[7] + 1;
auto Filename = ResolveImportedPath(PathBuf, Blob, ModuleDir);
+ StringRef FilenameAsRequested = Filename->substr(0, FilenameLen);
+ StringRef ExternalFilename = Filename->substr(FilenameLen);
shouldContinue = Listener.visitInputFile(
- *Filename, isSystemFile, Overridden, /*IsExplicitModule=*/false);
+ FilenameAsRequested, ExternalFilename, isSystemFile, Overridden,
+ /*IsExplicitModule=*/false);
break;
}
if (!shouldContinue)
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
index b7d44caca4954..3a6a8c25daa30 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -90,37 +90,98 @@ static bool checkHeaderSearchPaths(const HeaderSearchOptions &HSOpts,
using PrebuiltModuleFilesT = decltype(HeaderSearchOptions::PrebuiltModuleFiles);
-/// A listener that collects the imported modules and optionally the input
-/// files.
+/// A listener that collects the imported modules and the input
+/// files. While visiting, collect vfsoverlays and file inputs that determine
+/// whether prebuilt modules fully resolve in stable directories.
class PrebuiltModuleListener : public ASTReaderListener {
public:
PrebuiltModuleListener(PrebuiltModuleFilesT &PrebuiltModuleFiles,
llvm::SmallVector<std::string> &NewModuleFiles,
- PrebuiltModuleVFSMapT &PrebuiltModuleVFSMap,
+ PrebuiltModulesAttrsMap &PrebuiltModulesASTMap,
const HeaderSearchOptions &HSOpts,
- const LangOptions &LangOpts, DiagnosticsEngine &Diags)
+ const LangOptions &LangOpts, DiagnosticsEngine &Diags,
+ const llvm::SmallVector<StringRef> &StableDirs)
: PrebuiltModuleFiles(PrebuiltModuleFiles),
NewModuleFiles(NewModuleFiles),
- PrebuiltModuleVFSMap(PrebuiltModuleVFSMap), ExistingHSOpts(HSOpts),
- ExistingLangOpts(LangOpts), Diags(Diags) {}
+ PrebuiltModulesASTMap(PrebuiltModulesASTMap), ExistingHSOpts(HSOpts),
+ ExistingLangOpts(LangOpts), Diags(Diags), StableDirs(StableDirs) {}
bool needsImportVisitation() const override { return true; }
+ bool needsInputFileVisitation() override { return true; }
+ bool needsSystemInputFileVisitation() override { return true; }
+ /// Accumulate the modules are transitively depended on by the initial
+ /// prebuilt module.
void visitImport(StringRef ModuleName, StringRef Filename) override {
if (PrebuiltModuleFiles.insert({ModuleName.str(), Filename.str()}).second)
NewModuleFiles.push_back(Filename.str());
+
+ if (PrebuiltModulesASTMap.try_emplace(Filename).second)
+ PrebuiltModulesASTMap[Filename].setInStableDir(!StableDirs.empty());
+
+ if (auto It = PrebuiltModulesASTMap.find(CurrentFile);
+ It != PrebuiltModulesASTMap.end() && CurrentFile != Filename)
+ PrebuiltModulesASTMap[Filename].addDependent(It->getKey());
+ }
+
+ /// For each input file discovered, check whether it's external path is in a
+ /// stable directory. Traversal is stopped if the current module is not
+ /// considered stable.
+ bool visitInputFile(StringRef FilenameAsRequested, StringRef ExternalFilename,
+ bool isSystem, bool isOverridden,
+ bool isExplicitModule) override {
+ if (StableDirs.empty())
+ return false;
+ if (!PrebuiltModulesASTMap.contains(CurrentFile) ||
+ !PrebuiltModulesASTMap[CurrentFile].isInStableDir())
+ return false;
+
+ const StringRef FileToUse =
+ ExternalFilename.empty() ? FilenameAsRequested : ExternalFilename;
+
+ PrebuiltModulesASTMap[CurrentFile].setInStableDir(
+ isPathInStableDir(StableDirs, FileToUse));
+ return PrebuiltModulesASTMap[CurrentFile].isInStableDir();
}
+ /// Update which module that is being actively traversed.
void visitModuleFile(StringRef Filename,
serialization::ModuleKind Kind) override {
+ // If the CurrentFile is not
+ // considered stable, update any of it's transitive dependents.
+ if (PrebuiltModulesASTMap.contains(CurrentFile) &&
+ !PrebuiltModulesASTMap[CurrentFile].isInStableDir())
+ PrebuiltModulesASTMap[CurrentFile].updateDependentsNotInStableDirs(
+ PrebuiltModulesASTMap);
CurrentFile = Filename;
}
+ /// Check the header search options for a given module when considering
+ /// if the module comes from stable directories.
+ bool ReadHeaderSearchOptions(const HeaderSearchOptions &HSOpts,
+ StringRef ModuleFilename,
+ StringRef SpecificModuleCachePath,
+ bool Complain) override {
+ if (PrebuiltModulesASTMap.try_emplace(CurrentFile).second)
+ PrebuiltModulesASTMap[CurrentFile].setInStableDir(!StableDirs.empty());
+
+ if (PrebuiltModulesASTMap[CurrentFile].isInStableDir())
+ PrebuiltModulesASTMap[CurrentFile].setInStableDir(
+ areOptionsInStableDir(StableDirs, HSOpts));
+
+ return false;
+ }
+
+ /// Accumulate vfsoverlays used to build these prebuilt modules.
bool ReadHeaderSearchPaths(const HeaderSearchOptions &HSOpts,
bool Complain) override {
- std::vector<std::string> VFSOverlayFiles = HSOpts.VFSOverlayFiles;
- PrebuiltModuleVFSMap.try_emplace(CurrentFile, llvm::from_range,
- VFSOverlayFiles);
+
+ if (PrebuiltModulesASTMap.try_emplace(CurrentFile).second)
+ PrebuiltModulesASTMap[CurrentFile].setInStableDir(!StableDirs.empty());
+
+ PrebuiltModulesASTMap[CurrentFile].setVFS(
+ llvm::StringSet<>(llvm::from_range, HSOpts.VFSOverlayFiles));
+
return checkHeaderSearchPaths(
HSOpts, ExistingHSOpts, Complain ? &Diags : nullptr, ExistingLangOpts);
}
@@ -128,11 +189,12 @@ class PrebuiltModuleListener : public ASTReaderListener {
private:
PrebuiltModuleFilesT &PrebuiltModuleFiles;
llvm::SmallVector<std::string> &NewModuleFiles;
- PrebuiltModuleVFSMapT &PrebuiltModuleVFSMap;
+ PrebuiltModulesAttrsMap &PrebuiltModulesASTMap;
const HeaderSearchOptions &ExistingHSOpts;
const LangOptions &ExistingLangOpts;
DiagnosticsEngine &Diags;
std::string CurrentFile;
+ const llvm::SmallVector<StringRef> &StableDirs;
};
/// Visit the given prebuilt module and collect all of the modules it
@@ -140,13 +202,23 @@ class PrebuiltModuleListener : public ASTReaderListener {
static bool visitPrebuiltModule(StringRef PrebuiltModuleFilename,
CompilerInstance &CI,
PrebuiltModuleFilesT &ModuleFiles,
- PrebuiltModuleVFSMapT &PrebuiltModuleVFSMap,
+ PrebuiltModulesAttrsMap &PrebuiltModulesASTMap,
DiagnosticsEngine &Diags) {
+
+ // Gather the set of stable directories to use as transitive dependencies are
+ // discovered.
+ llvm::SmallVector<StringRef> StableDirs;
+ std::string SysrootToUse(CI.getHeaderSearchOpts().Sysroot);
+ if (!SysrootToUse.empty() &&
+ (llvm::sys::path::root_directory(SysrootToUse) != SysrootToUse))
+ StableDirs = {SysrootToUse, CI.getHeaderSearchOpts().ResourceDir};
+
// List of module files to be processed.
llvm::SmallVector<std::string> Worklist;
- PrebuiltModuleListener Listener(ModuleFiles, Worklist, PrebuiltModuleVFSMap,
+
+ PrebuiltModuleListener Listener(ModuleFiles, Worklist, PrebuiltModulesASTMap,
CI.getHeaderSearchOpts(), CI.getLangOpts(),
- Diags);
+ Diags, StableDirs);
Listener.visitModuleFile(PrebuiltModuleFilename,
serialization::MK_ExplicitModule);
@@ -371,16 +443,18 @@ class DependencyScanningAction : public tooling::ToolAction {
auto *FileMgr = ScanInstance.createFileManager(FS);
ScanInstance.createSourceManager(*FileMgr);
- // Store the list of prebuilt module files into header search options. This
- // will prevent the implicit build to create duplicate modules and will
- // force reuse of the existing prebuilt module files instead.
- PrebuiltModuleVFSMapT PrebuiltModuleVFSMap;
+ // Store a mapping of prebuilt module files and their properties like header
+ // search options. This will prevent the implicit build to create duplicate
+ // modules and will force reuse of the existing prebuilt module files
+ // instead.
+ PrebuiltModulesAttrsMap PrebuiltModulesASTMap;
+
if (!ScanInstance.getPreprocessorOpts().ImplicitPCHInclude.empty())
if (visitPrebuiltModule(
ScanInstance.getPreprocessorOpts().ImplicitPCHInclude,
ScanInstance,
ScanInstance.getHeaderSearchOpts().PrebuiltModuleFiles,
- PrebuiltModuleVFSMap, ScanInstance.getDiagnostics()))
+ PrebuiltModulesASTMap, ScanInstance.getDiagnostics()))
return false;
// Create the dependency collector that will collect the produced
@@ -410,7 +484,7 @@ class DependencyScanningAction : public tooling::ToolAction {
case ScanningOutputFormat::Full:
MDC = std::make_shared<ModuleDepCollector>(
Service, std::move(Opts), ScanInstance, Consumer, Controller,
- OriginalInvocation, std::move(PrebuiltModuleVFSMap));
+ OriginalInvocation, std::move(PrebuiltModulesASTMap));
ScanInstance.addDependencyCollector(MDC);
break;
}
diff --git a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
index 364f156566855..128007bde87bb 100644
--- a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -41,10 +41,20 @@ const std::vector<std::string> &ModuleDeps::getBuildArguments() const {
return std::get<std::vector<std::string>>(BuildInfo);
}
+void PrebuiltModuleASTAttrs::updateDependentsNotInStableDirs(
+ PrebuiltModulesAttrsMap &PrebuiltModulesMap) {
+ setInStableDir();
+ for (const auto Dep : ModuleFileDependents) {
+ if (!PrebuiltModulesMap[Dep].isInStableDir())
+ return;
+ PrebuiltModulesMap[Dep].updateDependentsNotInStableDirs(PrebuiltModulesMap);
+ }
+}
+
static void
optimizeHeaderSearchOpts(HeaderSearchOptions &Opts, ASTReader &Reader,
const serialization::ModuleFile &MF,
- const PrebuiltModuleVFSMapT &PrebuiltModuleVFSMap,
+ const PrebuiltModulesAttrsMap &PrebuiltModulesASTMap,
ScanningOptimizations OptimizeArgs) {
if (any(OptimizeArgs & ScanningOptimizations::HeaderSearch)) {
// Only preserve search paths that were used during the dependency scan.
@@ -89,11 +99,13 @@ optimizeHeaderSearchOpts(HeaderSearchOptions &Opts, ASTReader &Reader,
} else {
// This is not an implicitly built module, so it may have different
// VFS options. Fall back to a string comparison instead.
- auto VFSMap = PrebuiltModuleVFSMap.find(MF->FileName);
- if (VFSMap == PrebuiltModuleVFSMap.end())
+ auto PrebuiltModulePropIt =
+ PrebuiltModulesASTMap.find(MF->FileName);
+ if (PrebuiltModulePropIt == PrebuiltModulesASTMap.end())
return;
for (std::size_t I = 0, E = VFSOverlayFiles.size(); I != E; ++I) {
- if (VFSMap->second.contains(VFSOverlayFiles[I]))
+ if (PrebuiltModulePropIt->second.getVFS().contains(
+ VFSOverlayFiles[I]))
VFSUsage[I] = true;
}
}
@@ -159,32 +171,6 @@ static void optimizeCWD(CowCompilerInvocation &BuildInvocation, StringRef CWD) {
}
}
-/// Check a subset of invocation options to determine whether the current
-/// context can safely be considered as stable.
-static bool areOptionsInStableDir(CowCompilerInvocation &BuildInvocation,
- const ArrayRef<StringRef> StableDirs) {
- const auto &HSOpts = BuildInvocation.getHeaderSearchOpts();
- assert(isPathInStableDir(StableDirs, HSOpts.Sysroot) &&
- "Sysroots differ between module dependencies and current TU");
-
- assert(isPathInStableDir(StableDirs, HSOpts.ResourceDir) &&
- "ResourceDirs differ between module dependencies and current TU");
-
- for (const auto &Entry : HSOpts.UserEntries) {
- if (!Entry.IgnoreSysRoot)
- continue;
- if (!isPathInStableDir(StableDirs, Entry.Path))
- return false;
- }
-
- for (const auto &SysPrefix : HSOpts.SystemHeaderPrefixes) {
- if (!isPathInStableDir(StableDirs, SysPrefix.Prefix))
- return false;
- }
-
- return true;
-}
-
static std::vector<std::string> splitString(std::string S, char Separator) {
SmallVector<StringRef> Segments;
StringRef(S).split(Segments, Separator, /*MaxSplit=*/-1, /*KeepEmpty=*/false);
@@ -259,6 +245,29 @@ bool dependencies::isPathInStableDir(const ArrayRef<StringRef> Directories,
});
}
+bool dependencies::areOptionsInStableDir(const ArrayRef<StringRef> Directories,
+ const HeaderSearchOptions &HSOpts) {
+ assert(isPathInStableDir(Directories, HSOpts.Sysroot) &&
+ "Sysroots differ between module dependencies and current TU");
+
+ assert(isPathInStableDir(Directories, HSOpts.ResourceDir) &&
+ "ResourceDirs differ between module dependencies and current TU");
+
+ for (const auto &Entry : HSOpts.UserEntries) {
+ if (!Entry.IgnoreSysRoot)
+ continue;
+ if (!isPathInStableDir(Directories, Entry.Path))
+ return false;
+ }
+
+ for (const auto &SysPrefix : HSOpts.SystemHeaderPrefixes) {
+ if (!isPathInStableDir(Directories, SysPrefix.Prefix))
+ return false;
+ }
+
+ return true;
+}
+
static CowCompilerInvocation
makeCommonInvocationForModuleBuild(CompilerInvocation CI) {
CI.resetNonModularOptions();
@@ -827,7 +836,7 @@ ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {
ScanningOptimizations::VFS)))
optimizeHeaderSearchOpts(BuildInvocation.getMutHeaderSearchOpts(),
*MDC.ScanInstance.getASTReader(), *MF,
- MDC.PrebuiltModuleVFSMap,
+ MDC.PrebuiltModulesASTMap,
MDC.Service.getOptimizeArgs());
if (any(MDC.Service.getOptimizeArgs() &
@@ -851,7 +860,8 @@ ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {
// Check provided input paths from the invocation for determining
// IsInStableDirectories.
if (MD.IsInStableDirectories)
- MD.IsInStableDirectories = areOptionsInStableDir(CI, StableDirs);
+ MD.IsInStableDirectories =
+ areOptionsInStableDir(StableDirs, CI.getHeaderSearchOpts());
MDC.associateWithContextHash(CI, IgnoreCWD, MD);
@@ -896,10 +906,13 @@ void ModuleDepCollectorPP::addModulePrebuiltDeps(
if (MDC.isPrebuiltModule(Import->getTopLevelModule()))
if (SeenSubmodules.insert(Import->getTopLevelModule()).second) {
MD.PrebuiltModuleDeps.emplace_back(Import->getTopLevelModule());
- // Conservatively consider the module as not coming from stable
- // directories, as transitive dependencies from the prebuilt module
- // have not been determined.
- MD.IsInStableDirectories = false;
+ if (MD.IsInStableDirectories) {
+ auto PrebuiltModulePropIt = MDC.PrebuiltModulesASTMap.find(
+ MD.PrebuiltModuleDeps.back().PCMFile);
+ MD.IsInStableDirectories =
+ (PrebuiltModulePropIt != MDC.PrebuiltModulesASTMap.end()) &&
+ PrebuiltModulePropIt->second.isInStableDir();
+ }
}
}
@@ -962,10 +975,10 @@ ModuleDepCollector::ModuleDepCollector(
std::unique_ptr<DependencyOutputOptions> Opts,
CompilerInstance &ScanInstance, DependencyConsumer &C,
DependencyActionController &Controller, CompilerInvocation OriginalCI,
- PrebuiltModuleVFSMapT PrebuiltModuleVFSMap)
+ const PrebuiltModulesAttrsMap PrebuiltModulesASTMap)
: Service(Service), ScanInstance(ScanInstance), Consumer(C),
Controller(Controller),
- PrebuiltModuleVFSMap(std::move(PrebuiltModuleVFSMap)),
+ PrebuiltModulesASTMap(std::move(PrebuiltModulesASTMap)),
Opts(std::move(Opts)),
CommonInvocation(
makeCommonInvocationForModuleBuild(std::move(OriginalCI))) {}
diff --git a/clang/test/ClangScanDeps/prebuilt-modules-in-stable-dirs.c b/clang/test/ClangScanDeps/prebuilt-modules-in-stable-dirs.c
index 910d4890a9072..5e6a2a8bb42fa 100644
--- a/clang/test/ClangScanDeps/prebuilt-modules-in-stable-dirs.c
+++ b/clang/test/ClangScanDeps/prebuilt-modules-in-stable-dirs.c
@@ -1,4 +1,10 @@
-/// This test validates that modules that depend on prebuilt modules resolve `is-in-stable-directories` as false.
+/// This test validates that modules that depend on prebuilt modules
+/// resolve `is-in-stable-directories` correctly.
+/// The steps are:
+/// 1. Scan dependencies to build the PCH. One of the module's depend on header
+/// that is seemingly from the sysroot. However, it depends on a local header that is overlaid..
+/// 2. Build the PCH & dependency PCMs.
+/// 3. Scan a source file that transitively depends on the same modules as the pcm.
// REQUIRES: shell
// RUN: rm -rf %t
@@ -15,13 +21,19 @@
// RUN: clang-scan-deps -compilation-database %t/compile-commands.json \
// RUN: -j 1 -format experimental-full > %t/deps.db
// RUN: cat %t/deps_pch.db | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t --check-prefix PCH_DEP
-// RUN: cat %t/deps.db | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t
+// RUN: cat %t/deps.db | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t --check-prefix CLIENT
// PCH_DEP: "is-in-stable-directories": true
// PCH_DEP: "name": "A"
-
-// Verify is-in-stable-directories is not in any module dependencies, as they all depend on prebuilt modules.
-// CHECK-NOT: "is-in-stable-directories"
+
+// Verify is-in-stable-directories is only assigned to the module that only depends on A.
+// CLIENT-NOT: "is-in-stable-directories": true
+
+// CLIENT: "name": "D"
+// CLIENT: "is-in-stable-directories": true
+// CLIENT: "name": "sys"
+
+// CLIENT-NOT: "is-in-stable-directories": true
//--- compile-pch.json.in
[
@@ -74,6 +86,7 @@ module B [system] {
typedef int local_t;
//--- MacOSX.sdk/usr/include/sys/sys.h
+#include <A/A.h>
typedef int sys_t_m;
//--- MacOSX.sdk/usr/include/sys/module.modulemap
@@ -111,4 +124,5 @@ module D [system] {
#include <C/C.h> // This dependency transitively depends on a local header.
//--- client.c
+#include <sys/sys.h>
#include <D/D.h> // This dependency transitively depends on a local header.
>From 1dd2f868fd17814a95bd8864154519e04e65a846 Mon Sep 17 00:00:00 2001
From: Cyndy Ishida <cyndy_ishida at apple.com>
Date: Wed, 26 Mar 2025 08:37:54 -0700
Subject: [PATCH 2/3] Address review feedback
* Fix logic error when deserializing input file
* Also add a check for absolute paths in `isPathInStableDir`
* Always passdown filenameAsRequested & externalfilename
---
clang/include/clang/Serialization/ASTReader.h | 6 +--
clang/lib/Frontend/FrontendActions.cpp | 6 +--
clang/lib/Serialization/ASTReader.cpp | 46 +++++++++++++++----
.../DependencyScanningWorker.cpp | 5 +-
.../DependencyScanning/ModuleDepCollector.cpp | 11 +++--
.../prebuilt-modules-in-stable-dirs.c | 2 +-
6 files changed, 51 insertions(+), 25 deletions(-)
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index 617ac23984b60..c2fe07e727e06 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -242,9 +242,9 @@ class ASTReaderListener {
/// paths, for example when deserializing input files from AST files.
///
/// \returns true to continue receiving the next input file, false to stop.
- virtual bool visitInputFile(StringRef FilenameAsRequested, StringRef Filename,
- bool isSystem, bool isOverridden,
- bool isExplicitModule) {
+ virtual bool visitInputFile(StringRef FilenameAsRequested,
+ StringRef ExternalFilename, bool isSystem,
+ bool isOverridden, bool isExplicitModule) {
return true;
}
diff --git a/clang/lib/Frontend/FrontendActions.cpp b/clang/lib/Frontend/FrontendActions.cpp
index c5aeb92c7af73..30fdfbfade2c9 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -777,9 +777,9 @@ namespace {
/// Indicates that the AST file contains particular input file.
///
/// \returns true to continue receiving the next input file, false to stop.
- bool visitInputFile(StringRef FilenameAsRequested, StringRef Filename,
- bool isSystem, bool isOverridden,
- bool isExplicitModule) override {
+ bool visitInputFile(StringRef FilenameAsRequested,
+ StringRef ExternalFilename, bool isSystem,
+ bool isOverridden, bool isExplicitModule) override {
Out.indent(2) << "Input file: " << FilenameAsRequested;
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 9b78071ade4cd..13b1a3e7f3da0 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -2616,6 +2616,18 @@ bool ASTReader::shouldDisableValidationForFile(
return false;
}
+namespace {
+
+std::pair<StringRef, StringRef>
+getUnresolvedInputFilenames(const ASTReader::RecordData &Record,
+ const StringRef InputBlob) {
+ uint16_t AsRequestedLength = Record[7];
+ return {InputBlob.substr(0, AsRequestedLength),
+ InputBlob.substr(AsRequestedLength)};
+}
+
+} // namespace
+
InputFileInfo ASTReader::getInputFileInfo(ModuleFile &F, unsigned ID) {
// If this ID is bogus, just return an empty input file.
if (ID == 0 || ID > F.InputFileInfosLoaded.size())
@@ -2659,11 +2671,12 @@ InputFileInfo ASTReader::getInputFileInfo(ModuleFile &F, unsigned ID) {
R.Transient = static_cast<bool>(Record[4]);
R.TopLevel = static_cast<bool>(Record[5]);
R.ModuleMap = static_cast<bool>(Record[6]);
- uint16_t AsRequestedLength = Record[7];
- R.UnresolvedImportedFilenameAsRequested = Blob.substr(0, AsRequestedLength);
- R.UnresolvedImportedFilename = Blob.substr(AsRequestedLength);
- if (R.UnresolvedImportedFilename.empty())
- R.UnresolvedImportedFilename = R.UnresolvedImportedFilenameAsRequested;
+ auto [UnresolvedFilenameAsRequested, UnresolvedFilename] =
+ getUnresolvedInputFilenames(Record, Blob);
+ R.UnresolvedImportedFilenameAsRequested = UnresolvedFilenameAsRequested;
+ R.UnresolvedImportedFilename = UnresolvedFilename.empty()
+ ? UnresolvedFilenameAsRequested
+ : UnresolvedFilename;
Expected<llvm::BitstreamEntry> MaybeEntry = Cursor.advance();
if (!MaybeEntry) // FIXME this drops errors on the floor.
@@ -5704,6 +5717,11 @@ bool ASTReader::readASTFileControlBlock(
bool DoneWithControlBlock = false;
SmallString<0> PathBuf;
PathBuf.reserve(256);
+ // Additional path buffer to use when multiple paths need to be resolved.
+ // For example, when deserializing input files that contains a path that was
+ // resolved from a vfs overlay and an external location.
+ SmallString<0> AdditionalPathBuf;
+ AdditionalPathBuf.reserve(256);
while (!DoneWithControlBlock) {
Expected<llvm::BitstreamEntry> MaybeEntry = Stream.advance();
if (!MaybeEntry) {
@@ -5835,12 +5853,20 @@ bool ASTReader::readASTFileControlBlock(
break;
case INPUT_FILE:
bool Overridden = static_cast<bool>(Record[3]);
- size_t FilenameLen = ModuleDir.size() + Record[7] + 1;
- auto Filename = ResolveImportedPath(PathBuf, Blob, ModuleDir);
- StringRef FilenameAsRequested = Filename->substr(0, FilenameLen);
- StringRef ExternalFilename = Filename->substr(FilenameLen);
+ auto [UnresolvedFilenameAsRequested, UnresolvedFilename] =
+ getUnresolvedInputFilenames(Record, Blob);
+ auto FilenameAsRequestedBuf = ResolveImportedPath(
+ PathBuf, UnresolvedFilenameAsRequested, ModuleDir);
+ StringRef Filename;
+ if (UnresolvedFilename.empty())
+ Filename = *FilenameAsRequestedBuf;
+ else {
+ auto FilenameBuf = ResolveImportedPath(
+ AdditionalPathBuf, UnresolvedFilename, ModuleDir);
+ Filename = *FilenameBuf;
+ }
shouldContinue = Listener.visitInputFile(
- FilenameAsRequested, ExternalFilename, isSystemFile, Overridden,
+ *FilenameAsRequestedBuf, Filename, isSystemFile, Overridden,
/*IsExplicitModule=*/false);
break;
}
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
index 3a6a8c25daa30..9ab94b7a9adc5 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -136,11 +136,8 @@ class PrebuiltModuleListener : public ASTReaderListener {
!PrebuiltModulesASTMap[CurrentFile].isInStableDir())
return false;
- const StringRef FileToUse =
- ExternalFilename.empty() ? FilenameAsRequested : ExternalFilename;
-
PrebuiltModulesASTMap[CurrentFile].setInStableDir(
- isPathInStableDir(StableDirs, FileToUse));
+ isPathInStableDir(StableDirs, ExternalFilename));
return PrebuiltModulesASTMap[CurrentFile].isInStableDir();
}
diff --git a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
index 128007bde87bb..ebd392fbfa7d6 100644
--- a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -228,11 +228,14 @@ void dependencies::resetBenignCodeGenOptions(frontend::ActionKind ProgramAction,
bool dependencies::isPathInStableDir(const ArrayRef<StringRef> Directories,
const StringRef Input) {
+ using namespace llvm::sys;
+
+ if (!path::is_absolute(Input))
+ return false;
+
auto PathStartsWith = [](StringRef Prefix, StringRef Path) {
- auto PrefixIt = llvm::sys::path::begin(Prefix),
- PrefixEnd = llvm::sys::path::end(Prefix);
- for (auto PathIt = llvm::sys::path::begin(Path),
- PathEnd = llvm::sys::path::end(Path);
+ auto PrefixIt = path::begin(Prefix), PrefixEnd = path::end(Prefix);
+ for (auto PathIt = path::begin(Path), PathEnd = path::end(Path);
PrefixIt != PrefixEnd && PathIt != PathEnd; ++PrefixIt, ++PathIt) {
if (*PrefixIt != *PathIt)
return false;
diff --git a/clang/test/ClangScanDeps/prebuilt-modules-in-stable-dirs.c b/clang/test/ClangScanDeps/prebuilt-modules-in-stable-dirs.c
index 5e6a2a8bb42fa..bf9d178686abc 100644
--- a/clang/test/ClangScanDeps/prebuilt-modules-in-stable-dirs.c
+++ b/clang/test/ClangScanDeps/prebuilt-modules-in-stable-dirs.c
@@ -2,7 +2,7 @@
/// resolve `is-in-stable-directories` correctly.
/// The steps are:
/// 1. Scan dependencies to build the PCH. One of the module's depend on header
-/// that is seemingly from the sysroot. However, it depends on a local header that is overlaid..
+/// that is seemingly from the sysroot. However, it depends on a local header that is overlaid.
/// 2. Build the PCH & dependency PCMs.
/// 3. Scan a source file that transitively depends on the same modules as the pcm.
>From c43bd6a995ce95f180b3ed21ff303c08b9fe9918 Mon Sep 17 00:00:00 2001
From: Cyndy Ishida <cyndy_ishida at apple.com>
Date: Tue, 8 Apr 2025 10:26:03 -0700
Subject: [PATCH 3/3] fix nits
---
clang/include/clang/Serialization/ASTReader.h | 11 +++--
clang/lib/Frontend/FrontendActions.cpp | 6 +--
clang/lib/Serialization/ASTReader.cpp | 6 +--
.../DependencyScanningWorker.cpp | 48 +++++++++++--------
4 files changed, 38 insertions(+), 33 deletions(-)
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index c2fe07e727e06..2765c827ece2b 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -238,13 +238,14 @@ class ASTReaderListener {
}
/// Overloaded member function of \c visitInputFile that should
- /// be defined when the input file contains both the virtual and external
- /// paths, for example when deserializing input files from AST files.
+ /// be defined when there is a distinction between
+ /// the file name and name-as-requested. For example, when deserializing input
+ /// files from precompiled AST files.
///
/// \returns true to continue receiving the next input file, false to stop.
- virtual bool visitInputFile(StringRef FilenameAsRequested,
- StringRef ExternalFilename, bool isSystem,
- bool isOverridden, bool isExplicitModule) {
+ virtual bool visitInputFile(StringRef FilenameAsRequested, StringRef Filename,
+ bool isSystem, bool isOverridden,
+ bool isExplicitModule) {
return true;
}
diff --git a/clang/lib/Frontend/FrontendActions.cpp b/clang/lib/Frontend/FrontendActions.cpp
index 30fdfbfade2c9..c5aeb92c7af73 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -777,9 +777,9 @@ namespace {
/// Indicates that the AST file contains particular input file.
///
/// \returns true to continue receiving the next input file, false to stop.
- bool visitInputFile(StringRef FilenameAsRequested,
- StringRef ExternalFilename, bool isSystem,
- bool isOverridden, bool isExplicitModule) override {
+ bool visitInputFile(StringRef FilenameAsRequested, StringRef Filename,
+ bool isSystem, bool isOverridden,
+ bool isExplicitModule) override {
Out.indent(2) << "Input file: " << FilenameAsRequested;
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 13b1a3e7f3da0..753e83872d2dc 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -2616,9 +2616,7 @@ bool ASTReader::shouldDisableValidationForFile(
return false;
}
-namespace {
-
-std::pair<StringRef, StringRef>
+static std::pair<StringRef, StringRef>
getUnresolvedInputFilenames(const ASTReader::RecordData &Record,
const StringRef InputBlob) {
uint16_t AsRequestedLength = Record[7];
@@ -2626,8 +2624,6 @@ getUnresolvedInputFilenames(const ASTReader::RecordData &Record,
InputBlob.substr(AsRequestedLength)};
}
-} // namespace
-
InputFileInfo ASTReader::getInputFileInfo(ModuleFile &F, unsigned ID) {
// If this ID is bogus, just return an empty input file.
if (ID == 0 || ID > F.InputFileInfosLoaded.size())
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
index 9ab94b7a9adc5..6595f8ff5dc55 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -116,29 +116,32 @@ class PrebuiltModuleListener : public ASTReaderListener {
if (PrebuiltModuleFiles.insert({ModuleName.str(), Filename.str()}).second)
NewModuleFiles.push_back(Filename.str());
- if (PrebuiltModulesASTMap.try_emplace(Filename).second)
- PrebuiltModulesASTMap[Filename].setInStableDir(!StableDirs.empty());
+ auto PrebuiltMapEntry = PrebuiltModulesASTMap.try_emplace(Filename);
+ PrebuiltModuleASTAttrs &PrebuiltModule = PrebuiltMapEntry.first->second;
+ if (PrebuiltMapEntry.second)
+ PrebuiltModule.setInStableDir(!StableDirs.empty());
if (auto It = PrebuiltModulesASTMap.find(CurrentFile);
It != PrebuiltModulesASTMap.end() && CurrentFile != Filename)
- PrebuiltModulesASTMap[Filename].addDependent(It->getKey());
+ PrebuiltModule.addDependent(It->getKey());
}
/// For each input file discovered, check whether it's external path is in a
/// stable directory. Traversal is stopped if the current module is not
/// considered stable.
- bool visitInputFile(StringRef FilenameAsRequested, StringRef ExternalFilename,
+ bool visitInputFile(StringRef FilenameAsRequested, StringRef Filename,
bool isSystem, bool isOverridden,
bool isExplicitModule) override {
if (StableDirs.empty())
return false;
- if (!PrebuiltModulesASTMap.contains(CurrentFile) ||
- !PrebuiltModulesASTMap[CurrentFile].isInStableDir())
+ auto PrebuiltEntryIt = PrebuiltModulesASTMap.find(CurrentFile);
+ if ((PrebuiltEntryIt == PrebuiltModulesASTMap.end()) ||
+ (!PrebuiltEntryIt->second.isInStableDir()))
return false;
- PrebuiltModulesASTMap[CurrentFile].setInStableDir(
- isPathInStableDir(StableDirs, ExternalFilename));
- return PrebuiltModulesASTMap[CurrentFile].isInStableDir();
+ PrebuiltEntryIt->second.setInStableDir(
+ isPathInStableDir(StableDirs, Filename));
+ return PrebuiltEntryIt->second.isInStableDir();
}
/// Update which module that is being actively traversed.
@@ -146,9 +149,10 @@ class PrebuiltModuleListener : public ASTReaderListener {
serialization::ModuleKind Kind) override {
// If the CurrentFile is not
// considered stable, update any of it's transitive dependents.
- if (PrebuiltModulesASTMap.contains(CurrentFile) &&
- !PrebuiltModulesASTMap[CurrentFile].isInStableDir())
- PrebuiltModulesASTMap[CurrentFile].updateDependentsNotInStableDirs(
+ auto PrebuiltEntryIt = PrebuiltModulesASTMap.find(CurrentFile);
+ if ((PrebuiltEntryIt != PrebuiltModulesASTMap.end()) &&
+ !PrebuiltEntryIt->second.isInStableDir())
+ PrebuiltEntryIt->second.updateDependentsNotInStableDirs(
PrebuiltModulesASTMap);
CurrentFile = Filename;
}
@@ -159,12 +163,14 @@ class PrebuiltModuleListener : public ASTReaderListener {
StringRef ModuleFilename,
StringRef SpecificModuleCachePath,
bool Complain) override {
- if (PrebuiltModulesASTMap.try_emplace(CurrentFile).second)
- PrebuiltModulesASTMap[CurrentFile].setInStableDir(!StableDirs.empty());
- if (PrebuiltModulesASTMap[CurrentFile].isInStableDir())
- PrebuiltModulesASTMap[CurrentFile].setInStableDir(
- areOptionsInStableDir(StableDirs, HSOpts));
+ auto PrebuiltMapEntry = PrebuiltModulesASTMap.try_emplace(CurrentFile);
+ PrebuiltModuleASTAttrs &PrebuiltModule = PrebuiltMapEntry.first->second;
+ if (PrebuiltMapEntry.second)
+ PrebuiltModule.setInStableDir(!StableDirs.empty());
+
+ if (PrebuiltModule.isInStableDir())
+ PrebuiltModule.setInStableDir(areOptionsInStableDir(StableDirs, HSOpts));
return false;
}
@@ -173,10 +179,12 @@ class PrebuiltModuleListener : public ASTReaderListener {
bool ReadHeaderSearchPaths(const HeaderSearchOptions &HSOpts,
bool Complain) override {
- if (PrebuiltModulesASTMap.try_emplace(CurrentFile).second)
- PrebuiltModulesASTMap[CurrentFile].setInStableDir(!StableDirs.empty());
+ auto PrebuiltMapEntry = PrebuiltModulesASTMap.try_emplace(CurrentFile);
+ PrebuiltModuleASTAttrs &PrebuiltModule = PrebuiltMapEntry.first->second;
+ if (PrebuiltMapEntry.second)
+ PrebuiltModule.setInStableDir(!StableDirs.empty());
- PrebuiltModulesASTMap[CurrentFile].setVFS(
+ PrebuiltModule.setVFS(
llvm::StringSet<>(llvm::from_range, HSOpts.VFSOverlayFiles));
return checkHeaderSearchPaths(
More information about the cfe-commits
mailing list