Skip to content

[clang][ScanDeps] Allow PCHs to have different VFS overlays #82294

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 24, 2024
Merged

[clang][ScanDeps] Allow PCHs to have different VFS overlays #82294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion clang/include/clang/Basic/DiagnosticSerializationKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ def err_pch_diagopt_mismatch : Error<"%0 is currently enabled, but was not in "
"the PCH file">;
def err_pch_modulecache_mismatch : Error<"PCH was compiled with module cache "
"path '%0', but the path is currently '%1'">;
def err_pch_vfsoverlay_mismatch : Error<"PCH was compiled with different VFS overlay files than are currently in use">;
def warn_pch_vfsoverlay_mismatch : Warning<
"PCH was compiled with different VFS overlay files than are currently in use">,
InGroup<DiagGroup<"pch-vfs-diff">>;
def note_pch_vfsoverlay_files : Note<"%select{PCH|current translation unit}0 has the following VFS overlays:\n%1">;
def note_pch_vfsoverlay_empty : Note<"%select{PCH|current translation unit}0 has no VFS overlays">;

Expand Down
5 changes: 5 additions & 0 deletions clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,8 @@ struct ModuleDeps {
BuildInfo;
};

using PrebuiltModuleVFSMapT = llvm::StringMap<llvm::StringSet<>>;

class ModuleDepCollector;

/// Callback that records textual includes and direct modular includes/imports
Expand Down Expand Up @@ -214,6 +216,7 @@ class ModuleDepCollector final : public DependencyCollector {
CompilerInstance &ScanInstance, DependencyConsumer &C,
DependencyActionController &Controller,
CompilerInvocation OriginalCI,
PrebuiltModuleVFSMapT PrebuiltModuleVFSMap,
ScanningOptimizations OptimizeArgs, bool EagerLoadModules,
bool IsStdModuleP1689Format);

Expand All @@ -233,6 +236,8 @@ 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;
/// Path to the main source file.
std::string MainFile;
/// Hash identifying the compilation conditions of the current TU.
Expand Down
58 changes: 47 additions & 11 deletions clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "clang/Tooling/DependencyScanning/DependencyScanningService.h"
#include "clang/Tooling/DependencyScanning/ModuleDepCollector.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/ScopeExit.h"
#include "llvm/Support/Allocator.h"
#include "llvm/Support/Error.h"
#include "llvm/TargetParser/Host.h"
Expand Down Expand Up @@ -67,7 +68,7 @@ static bool checkHeaderSearchPaths(const HeaderSearchOptions &HSOpts,
if (LangOpts.Modules) {
if (HSOpts.VFSOverlayFiles != ExistingHSOpts.VFSOverlayFiles) {
if (Diags) {
Diags->Report(diag::err_pch_vfsoverlay_mismatch);
Diags->Report(diag::warn_pch_vfsoverlay_mismatch);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this is applicable outside of the scanner. Should we move this into the generic PCH loading code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, this is currently disabled by default, but is useful for normal PCH too. It would be an issue if we ever want to start changing the original command lines too though (to increase cache hits for normal TUs), there you only want to warn when scanning, not in the actual build.

auto VFSNote = [&](int Type, ArrayRef<std::string> VFSOverlays) {
if (VFSOverlays.empty()) {
Diags->Report(diag::note_pch_vfsoverlay_empty) << Type;
Expand All @@ -79,7 +80,6 @@ static bool checkHeaderSearchPaths(const HeaderSearchOptions &HSOpts,
VFSNote(0, HSOpts.VFSOverlayFiles);
VFSNote(1, ExistingHSOpts.VFSOverlayFiles);
}
return true;
}
}
return false;
Expand All @@ -93,10 +93,12 @@ class PrebuiltModuleListener : public ASTReaderListener {
public:
PrebuiltModuleListener(PrebuiltModuleFilesT &PrebuiltModuleFiles,
llvm::SmallVector<std::string> &NewModuleFiles,
PrebuiltModuleVFSMapT &PrebuiltModuleVFSMap,
const HeaderSearchOptions &HSOpts,
const LangOptions &LangOpts, DiagnosticsEngine &Diags)
: PrebuiltModuleFiles(PrebuiltModuleFiles),
NewModuleFiles(NewModuleFiles), ExistingHSOpts(HSOpts),
NewModuleFiles(NewModuleFiles),
PrebuiltModuleVFSMap(PrebuiltModuleVFSMap), ExistingHSOpts(HSOpts),
ExistingLangOpts(LangOpts), Diags(Diags) {}

bool needsImportVisitation() const override { return true; }
Expand All @@ -106,31 +108,45 @@ class PrebuiltModuleListener : public ASTReaderListener {
NewModuleFiles.push_back(Filename.str());
}

void visitModuleFile(StringRef Filename,
serialization::ModuleKind Kind) override {
CurrentFile = Filename;
}

bool ReadHeaderSearchPaths(const HeaderSearchOptions &HSOpts,
bool Complain) override {
std::vector<std::string> VFSOverlayFiles = HSOpts.VFSOverlayFiles;
PrebuiltModuleVFSMap.insert(
{CurrentFile, llvm::StringSet<>(VFSOverlayFiles)});
return checkHeaderSearchPaths(
HSOpts, ExistingHSOpts, Complain ? &Diags : nullptr, ExistingLangOpts);
}

private:
PrebuiltModuleFilesT &PrebuiltModuleFiles;
llvm::SmallVector<std::string> &NewModuleFiles;
PrebuiltModuleVFSMapT &PrebuiltModuleVFSMap;
const HeaderSearchOptions &ExistingHSOpts;
const LangOptions &ExistingLangOpts;
DiagnosticsEngine &Diags;
std::string CurrentFile;
};

/// Visit the given prebuilt module and collect all of the modules it
/// transitively imports and contributing input files.
static bool visitPrebuiltModule(StringRef PrebuiltModuleFilename,
CompilerInstance &CI,
PrebuiltModuleFilesT &ModuleFiles,
PrebuiltModuleVFSMapT &PrebuiltModuleVFSMap,
DiagnosticsEngine &Diags) {
// List of module files to be processed.
llvm::SmallVector<std::string> Worklist;
PrebuiltModuleListener Listener(
ModuleFiles, Worklist, CI.getHeaderSearchOpts(), CI.getLangOpts(), Diags);
PrebuiltModuleListener Listener(ModuleFiles, Worklist, PrebuiltModuleVFSMap,
CI.getHeaderSearchOpts(), CI.getLangOpts(),
Diags);

Listener.visitModuleFile(PrebuiltModuleFilename,
serialization::MK_ExplicitModule);
if (ASTReader::readASTFileControlBlock(
PrebuiltModuleFilename, CI.getFileManager(), CI.getModuleCache(),
CI.getPCHContainerReader(),
Expand All @@ -139,6 +155,7 @@ static bool visitPrebuiltModule(StringRef PrebuiltModuleFilename,
return true;

while (!Worklist.empty()) {
Listener.visitModuleFile(Worklist.back(), serialization::MK_ExplicitModule);
if (ASTReader::readASTFileControlBlock(
Worklist.pop_back_val(), CI.getFileManager(), CI.getModuleCache(),
CI.getPCHContainerReader(),
Expand Down Expand Up @@ -175,8 +192,19 @@ static void sanitizeDiagOpts(DiagnosticOptions &DiagOpts) {
DiagOpts.ShowCarets = false;
// Don't write out diagnostic file.
DiagOpts.DiagnosticSerializationFile.clear();
// Don't emit warnings as errors (and all other warnings too).
DiagOpts.IgnoreWarnings = true;
// Don't emit warnings except for scanning specific warnings.
// TODO: It would be useful to add a more principled way to ignore all
// warnings that come from source code. The issue is that we need to
// ignore warnings that could be surpressed by
// `#pragma clang diagnostic`, while still allowing some scanning
// warnings for things we're not ready to turn into errors yet.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned about removing IngoreWarnings = true. There are lots of default warnings we may not want. Also this would allow #pragma clang diagnostic to promote arbitrary diagnostics to errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scanner never sees #pragma clang diagnostic, so there's no issue with code that uses that to turn warnings on. The original issue was with warnings getting turned off via #pragma clang diagnostic, but the new code removes all warnings and Werrors, so you're just left with default warnings.

The goal here was to keep driver warnings (which are lost otherwise) and allow us to have dedicated scanner warnings. I do think we want more control over this, possibly add a scanner bit to diagnostics so we can be explicit about which warnings we expect from the scanner, but I think this change is fine for now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scanner never sees #pragma clang diagnostic, so there's no issue with code that uses that to turn warnings on.

Ah sorry, I forgot we skipped over most pragmas.

so you're just left with default warnings.

The goal here was to keep driver warnings (which are lost otherwise) and allow us to have dedicated scanner warnings. I do think we want more control over this, possibly add a scanner bit to diagnostics so we can be explicit about which warnings we expect from the scanner, but I think this change is fine for now.

This goal makes some sense to me, but I'm not sure that using the default warnings are a good idea. The default warnings can just as easily cause us to emit a driver warning that the user was explicitly trying to hide.

// See `test/ClangScanDeps/diagnostic-pragmas.c` for an example.
llvm::erase_if(DiagOpts.Warnings, [](StringRef Warning) {
return llvm::StringSwitch<bool>(Warning)
.Cases("pch-vfs-diff", "error=pch-vfs-diff", false)
.StartsWith("no-error=", false)
.Default(true);
});
}

/// A clang tool that runs the preprocessor in a mode that's optimized for
Expand Down Expand Up @@ -226,14 +254,19 @@ class DependencyScanningAction : public tooling::ToolAction {
if (!ScanInstance.hasDiagnostics())
return false;

// Some DiagnosticConsumers require that finish() is called.
auto DiagConsumerFinisher =
llvm::make_scope_exit([DiagConsumer]() { DiagConsumer->finish(); });

ScanInstance.getPreprocessorOpts().AllowPCHWithDifferentModulesCachePath =
true;

ScanInstance.getFrontendOpts().GenerateGlobalModuleIndex = false;
ScanInstance.getFrontendOpts().UseGlobalModuleIndex = false;
ScanInstance.getFrontendOpts().ModulesShareFileManager = false;
ScanInstance.getHeaderSearchOpts().ModuleFormat = "raw";
ScanInstance.getHeaderSearchOpts().ModulesIncludeVFSUsage = true;
ScanInstance.getHeaderSearchOpts().ModulesIncludeVFSUsage =
any(OptimizeArgs & ScanningOptimizations::VFS);

ScanInstance.setFileManager(FileMgr);
// Support for virtual file system overlays.
Expand All @@ -246,12 +279,13 @@ class DependencyScanningAction : public tooling::ToolAction {
// 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;
if (!ScanInstance.getPreprocessorOpts().ImplicitPCHInclude.empty())
if (visitPrebuiltModule(
ScanInstance.getPreprocessorOpts().ImplicitPCHInclude,
ScanInstance,
ScanInstance.getHeaderSearchOpts().PrebuiltModuleFiles,
ScanInstance.getDiagnostics()))
PrebuiltModuleVFSMap, ScanInstance.getDiagnostics()))
return false;

// Use the dependency scanning optimized file system if requested to do so.
Expand Down Expand Up @@ -295,8 +329,8 @@ class DependencyScanningAction : public tooling::ToolAction {
case ScanningOutputFormat::Full:
MDC = std::make_shared<ModuleDepCollector>(
std::move(Opts), ScanInstance, Consumer, Controller,
OriginalInvocation, OptimizeArgs, EagerLoadModules,
Format == ScanningOutputFormat::P1689);
OriginalInvocation, std::move(PrebuiltModuleVFSMap), OptimizeArgs,
EagerLoadModules, Format == ScanningOutputFormat::P1689);
ScanInstance.addDependencyCollector(MDC);
break;
}
Expand Down Expand Up @@ -325,6 +359,8 @@ class DependencyScanningAction : public tooling::ToolAction {
if (ScanInstance.getDiagnostics().hasErrorOccurred())
return false;

// Each action is responsible for calling finish.
DiagConsumerFinisher.release();
const bool Result = ScanInstance.ExecuteAction(*Action);

if (Result)
Expand Down
34 changes: 26 additions & 8 deletions clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,11 @@ const std::vector<std::string> &ModuleDeps::getBuildArguments() {
return std::get<std::vector<std::string>>(BuildInfo);
}

static void optimizeHeaderSearchOpts(HeaderSearchOptions &Opts,
ASTReader &Reader,
const serialization::ModuleFile &MF,
ScanningOptimizations OptimizeArgs) {
static void
optimizeHeaderSearchOpts(HeaderSearchOptions &Opts, ASTReader &Reader,
const serialization::ModuleFile &MF,
const PrebuiltModuleVFSMapT &PrebuiltModuleVFSMap,
ScanningOptimizations OptimizeArgs) {
if (any(OptimizeArgs & ScanningOptimizations::HeaderSearch)) {
// Only preserve search paths that were used during the dependency scan.
std::vector<HeaderSearchOptions::Entry> Entries;
Expand Down Expand Up @@ -65,11 +66,25 @@ static void optimizeHeaderSearchOpts(HeaderSearchOptions &Opts,
llvm::DenseSet<const serialization::ModuleFile *> Visited;
std::function<void(const serialization::ModuleFile *)> VisitMF =
[&](const serialization::ModuleFile *MF) {
VFSUsage |= MF->VFSUsage;
Visited.insert(MF);
for (const serialization::ModuleFile *Import : MF->Imports)
if (!Visited.contains(Import))
VisitMF(Import);
if (MF->Kind == serialization::MK_ImplicitModule) {
VFSUsage |= MF->VFSUsage;
// We only need to recurse into implicit modules. Other module types
// will have the correct set of VFSs for anything they depend on.
for (const serialization::ModuleFile *Import : MF->Imports)
if (!Visited.contains(Import))
VisitMF(Import);
} 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())
return;
Copy link
Contributor

@jansvoboda11 jansvoboda11 Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This almost makes it seem like it's okay to import a non-implicit module that we didn't visit when dealing with the PCH. I think it would be good to add a sanity check here by having an entry for each PCH dependency (that will map to empty set if no VFS overlay files were used) and assert here that VFSMap != PrebuiltModuleVFSMap.end().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's also possible that you used an explicit module in the original driver command, or prebuilt module path. The idea of silently ignoring unknown modules here was to preserve the existing behavior of ignoring them. I would eventually like to make this an error, but doing this works in most cases now, so I don't want to do that yet. It would probably be good to make it a warning in this patch though, I don't expect this return to fire unless something weird is going on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still like this implemented in this PR, but not mandatory.

for (std::size_t I = 0, E = VFSOverlayFiles.size(); I != E; ++I) {
if (VFSMap->second.contains(VFSOverlayFiles[I]))
VFSUsage[I] = true;
}
}
};
VisitMF(&MF);

Expand Down Expand Up @@ -596,6 +611,7 @@ ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {
ScanningOptimizations::VFS)))
optimizeHeaderSearchOpts(BuildInvocation.getMutHeaderSearchOpts(),
*MDC.ScanInstance.getASTReader(), *MF,
MDC.PrebuiltModuleVFSMap,
MDC.OptimizeArgs);
if (any(MDC.OptimizeArgs & ScanningOptimizations::SystemWarnings))
optimizeDiagnosticOpts(
Expand Down Expand Up @@ -697,9 +713,11 @@ ModuleDepCollector::ModuleDepCollector(
std::unique_ptr<DependencyOutputOptions> Opts,
CompilerInstance &ScanInstance, DependencyConsumer &C,
DependencyActionController &Controller, CompilerInvocation OriginalCI,
PrebuiltModuleVFSMapT PrebuiltModuleVFSMap,
ScanningOptimizations OptimizeArgs, bool EagerLoadModules,
bool IsStdModuleP1689Format)
: ScanInstance(ScanInstance), Consumer(C), Controller(Controller),
PrebuiltModuleVFSMap(std::move(PrebuiltModuleVFSMap)),
Opts(std::move(Opts)),
CommonInvocation(
makeCommonInvocationForModuleBuild(std::move(OriginalCI))),
Expand Down
Loading