Thanks for the feedback!
Argyrios Kyrtzidis <[email protected]> writes:
> Thank you for your work on this.
>
> Some high level comments first; the way ModuleDependencyCollector is
> getting hooked up, results in:
>
> - If no module needs building (because module cache has it) no module
> dependency is getting written out
I realize this, but as far as I can tell there isn't really a practical
way to get all of the module dependencies except for when we actually
create a module. Unless there's a way to extract this information from
an imported ModuleFile that I've missed, I think we need to collect this
information as we parse the AST to build a module in the first place.
If there is a more reliable way to do this, I'm all ears. I should
mention though, for the purposes of getting more useful crash reports
with modules this limitation isn't very important. Notably, it's useful
to collect the .pcm files anyway, and using a fresh cache directory is a
very simple way to do that.
> - It doesn’t hook in the top level module, e.g. if I have "@import
> Darwin;” then no module dependency is written out even if the module
> cache is empty.
This seems like a bigger problem, and I don't really understand it.
Where is the module built? I'm pretty convinced that the place I've
hooked into is the only one that creates a CompilerInstance with
BuildingModule set to true...
> Some nitpicks:
>
> @@ -105,6 +105,9 @@ class CompilerInstance : public ModuleLoader {
> /// \brief The ASTReader, if one exists.
> IntrusiveRefCntPtr<ASTReader> ModuleManager;
>
> + /// \brief The module dependency collector for crashdumps
> + std::shared_ptr<ModuleDependencyCollector> ModuleDepCollector;
> +
> /// \brief The dependency file generator.
> std::unique_ptr<DependencyFileGenerator> TheDependencyFileGenerator;
>
> Is there a policy to avoid IntrusiveRefCntPtr ?
Since we're using C++11 now, I just assumed we'd be migrating to
shared_ptr where appropriate in modern code. I can change it to
IntrusiveRefCntPtr if you like, but I do think shared_ptr is appropriate
here.
> +/// Collects the dependencies for imported modules into a directory. Users
> +/// should attach to the AST reader whenever a module is loaded.
> +class ModuleDependencyCollector {
> + StringRef DestDir;
>
> Use std::string here.
Sure.
> + void reportError() { HasErrors = true; }
>
> Name is a bit misleading, how about "setHasErrors()” ?
Good idea.
> +llvm::error_code ModuleDependencyListener::copyToRoot(StringRef Src) {
> + using namespace llvm::sys;
> + SmallString<256> Dest = Collector.getDest();
> + path::append(Dest, path::relative_path(Src));
>
> Do we need to turn Src into an absolute path first, in case it’s
> relative to the current directory ?
Yes, I think so.
> + if (llvm::error_code EC = fs::copy_file(Src, Dest.str()))
> + return EC;
>
> Do you have a separate patch that introduces copy_file ?
Oops, forgot to attach that!
Updated patches attached.
>From a151ebfb00b2525b1debc340758d0a6a07b15bb6 Mon Sep 17 00:00:00 2001
From: Justin Bogner <[email protected]>
Date: Mon, 12 May 2014 23:16:57 -0700
Subject: [PATCH 1/2] Frontend: Add a CC1 flag to dump module dependencies to a
directory
This adds the -module-dependency-dir to clang -cc1, which specifies a
directory to copy all of a module's dependencies into in a form
suitable to be used as a VFS. This is useful for crashdumps that
involve modules, so that the module dependencies will be intact when a
crash report script is used to reproduce a problem on another machine.
---
include/clang/Driver/Options.td | 2 +
include/clang/Frontend/CompilerInstance.h | 7 +++
include/clang/Frontend/DependencyOutputOptions.h | 5 +-
include/clang/Frontend/Utils.h | 19 +++++++
lib/Frontend/CMakeLists.txt | 1 +
lib/Frontend/CompilerInstance.cpp | 26 ++++++++++
lib/Frontend/CompilerInvocation.cpp | 2 +
lib/Frontend/ModuleDependencyCollector.cpp | 64 ++++++++++++++++++++++++
8 files changed, 125 insertions(+), 1 deletion(-)
create mode 100644 lib/Frontend/ModuleDependencyCollector.cpp
diff --git a/include/clang/Driver/Options.td b/include/clang/Driver/Options.td
index 85f2e0c..dd8722e 100644
--- a/include/clang/Driver/Options.td
+++ b/include/clang/Driver/Options.td
@@ -342,6 +342,8 @@ def dependency_file : Separate<["-"], "dependency-file">, Flags<[CC1Option]>,
HelpText<"Filename (or -) to write dependency output to">;
def dependency_dot : Separate<["-"], "dependency-dot">, Flags<[CC1Option]>,
HelpText<"Filename to write DOT-formatted header dependencies to">;
+def module_dependency_dir : Separate<["-"], "module-dependency-dir">,
+ Flags<[CC1Option]>, HelpText<"Directory to dump module dependencies to">;
def dumpmachine : Flag<["-"], "dumpmachine">;
def dumpspecs : Flag<["-"], "dumpspecs">, Flags<[Unsupported]>;
def dumpversion : Flag<["-"], "dumpversion">;
diff --git a/include/clang/Frontend/CompilerInstance.h b/include/clang/Frontend/CompilerInstance.h
index b144069..d72f904 100644
--- a/include/clang/Frontend/CompilerInstance.h
+++ b/include/clang/Frontend/CompilerInstance.h
@@ -105,6 +105,9 @@ class CompilerInstance : public ModuleLoader {
/// \brief The ASTReader, if one exists.
IntrusiveRefCntPtr<ASTReader> ModuleManager;
+ /// \brief The module dependency collector for crashdumps
+ std::shared_ptr<ModuleDependencyCollector> ModuleDepCollector;
+
/// \brief The dependency file generator.
std::unique_ptr<DependencyFileGenerator> TheDependencyFileGenerator;
@@ -464,6 +467,10 @@ public:
IntrusiveRefCntPtr<ASTReader> getModuleManager() const;
void setModuleManager(IntrusiveRefCntPtr<ASTReader> Reader);
+ std::shared_ptr<ModuleDependencyCollector> getModuleDepCollector() const;
+ void setModuleDepCollector(
+ std::shared_ptr<ModuleDependencyCollector> Collector);
+
/// }
/// @name Code Completion
/// {
diff --git a/include/clang/Frontend/DependencyOutputOptions.h b/include/clang/Frontend/DependencyOutputOptions.h
index d275249..5da1459 100644
--- a/include/clang/Frontend/DependencyOutputOptions.h
+++ b/include/clang/Frontend/DependencyOutputOptions.h
@@ -43,7 +43,10 @@ public:
/// \brief The file to write GraphViz-formatted header dependencies to.
std::string DOTOutputFile;
-
+
+ /// \brief The directory to copy module dependencies to when collecting them.
+ std::string ModuleDependencyOutputDir;
+
public:
DependencyOutputOptions() {
IncludeSystemHeaders = 0;
diff --git a/include/clang/Frontend/Utils.h b/include/clang/Frontend/Utils.h
index 8fb536f..f793e7c 100644
--- a/include/clang/Frontend/Utils.h
+++ b/include/clang/Frontend/Utils.h
@@ -17,6 +17,7 @@
#include "clang/Basic/Diagnostic.h"
#include "llvm/ADT/IntrusiveRefCntPtr.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
#include "llvm/Option/OptSpecifier.h"
namespace llvm {
@@ -80,6 +81,24 @@ public:
void AttachToASTReader(ASTReader &R);
};
+/// Collects the dependencies for imported modules into a directory. Users
+/// should attach to the AST reader whenever a module is loaded.
+class ModuleDependencyCollector {
+ std::string DestDir;
+ bool HasErrors;
+ llvm::StringSet<> Seen;
+
+public:
+ StringRef getDest() { return DestDir; }
+ bool insertSeen(StringRef Filename) { return Seen.insert(Filename); }
+ void setHasErrors() { HasErrors = true; }
+
+ void attachToASTReader(ASTReader &R);
+ bool hasErrors() { return HasErrors; }
+ ModuleDependencyCollector(std::string DestDir)
+ : DestDir(DestDir), HasErrors(false) {}
+};
+
/// AttachDependencyGraphGen - Create a dependency graph generator, and attach
/// it to the given preprocessor.
void AttachDependencyGraphGen(Preprocessor &PP, StringRef OutputFile,
diff --git a/lib/Frontend/CMakeLists.txt b/lib/Frontend/CMakeLists.txt
index b67e0ae..3fa7a2c 100644
--- a/lib/Frontend/CMakeLists.txt
+++ b/lib/Frontend/CMakeLists.txt
@@ -25,6 +25,7 @@ add_clang_library(clangFrontend
LangStandards.cpp
LayoutOverrideSource.cpp
LogDiagnosticPrinter.cpp
+ ModuleDependencyCollector.cpp
MultiplexConsumer.cpp
PrintPreprocessedOutput.cpp
SerializedDiagnosticPrinter.cpp
diff --git a/lib/Frontend/CompilerInstance.cpp b/lib/Frontend/CompilerInstance.cpp
index e8ca080..584b1ec 100644
--- a/lib/Frontend/CompilerInstance.cpp
+++ b/lib/Frontend/CompilerInstance.cpp
@@ -116,6 +116,25 @@ void CompilerInstance::setModuleManager(IntrusiveRefCntPtr<ASTReader> Reader) {
ModuleManager = Reader;
}
+std::shared_ptr<ModuleDependencyCollector>
+CompilerInstance::getModuleDepCollector() const {
+ if (ModuleDepCollector)
+ return ModuleDepCollector;
+
+ // If we don't have a collector, but we are collecting module dependencies,
+ // then we're the top level compiler instance and need to create one.
+ const DependencyOutputOptions &DepOpts = getDependencyOutputOpts();
+ if (DepOpts.ModuleDependencyOutputDir.empty())
+ return nullptr;
+ StringRef OutputDir = DepOpts.ModuleDependencyOutputDir;
+ return std::make_shared<ModuleDependencyCollector>(OutputDir);
+}
+
+void CompilerInstance::setModuleDepCollector(
+ std::shared_ptr<ModuleDependencyCollector> Collector) {
+ ModuleDepCollector = Collector;
+}
+
// Diagnostics
static void SetUpDiagnosticLog(DiagnosticOptions *DiagOpts,
const CodeGenOptions *CodeGenOpts,
@@ -851,6 +870,10 @@ static void compileModuleImpl(CompilerInstance &ImportingInstance,
SourceMgr.pushModuleBuildStack(Module->getTopLevelModuleName(),
FullSourceLoc(ImportLoc, ImportingInstance.getSourceManager()));
+ // If we're collecting module dependencies, we need to share a collector
+ // between all of the module CompilerInstances.
+ Instance.setModuleDepCollector(ImportingInstance.getModuleDepCollector());
+
// Get or create the module map that we'll use to build this module.
std::string InferredModuleMapContent;
if (const FileEntry *ModuleMapFile =
@@ -1211,6 +1234,9 @@ CompilerInstance::loadModule(SourceLocation ImportLoc,
if (TheDependencyFileGenerator)
TheDependencyFileGenerator->AttachToASTReader(*ModuleManager);
+ if (ModuleDepCollector)
+ ModuleDepCollector->attachToASTReader(*ModuleManager);
+
// Try to load the module file.
unsigned ARRFlags = ASTReader::ARR_OutOfDate | ASTReader::ARR_Missing;
switch (ModuleManager->ReadAST(ModuleFileName, serialization::MK_Module,
diff --git a/lib/Frontend/CompilerInvocation.cpp b/lib/Frontend/CompilerInvocation.cpp
index 6d5faf9..beaa092 100644
--- a/lib/Frontend/CompilerInvocation.cpp
+++ b/lib/Frontend/CompilerInvocation.cpp
@@ -578,6 +578,8 @@ static void ParseDependencyOutputArgs(DependencyOutputOptions &Opts,
Opts.AddMissingHeaderDeps = Args.hasArg(OPT_MG);
Opts.PrintShowIncludes = Args.hasArg(OPT_show_includes);
Opts.DOTOutputFile = Args.getLastArgValue(OPT_dependency_dot);
+ Opts.ModuleDependencyOutputDir =
+ Args.getLastArgValue(OPT_module_dependency_dir);
}
bool clang::ParseDiagnosticArgs(DiagnosticOptions &Opts, ArgList &Args,
diff --git a/lib/Frontend/ModuleDependencyCollector.cpp b/lib/Frontend/ModuleDependencyCollector.cpp
new file mode 100644
index 0000000..0d2bfc1
--- /dev/null
+++ b/lib/Frontend/ModuleDependencyCollector.cpp
@@ -0,0 +1,64 @@
+//===--- ModuleDependencyCollector.cpp - Collect module dependencies ------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// Collect the dependencies of a set of modules.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Frontend/Utils.h"
+#include "clang/Serialization/ASTReader.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/Support/Filesystem.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
+
+using namespace clang;
+
+namespace {
+/// Private implementation for ModuleDependencyCollector
+class ModuleDependencyListener : public ASTReaderListener {
+ ModuleDependencyCollector &Collector;
+
+ std::error_code copyToRoot(StringRef Src);
+public:
+ ModuleDependencyListener(ModuleDependencyCollector &Collector)
+ : Collector(Collector) {}
+ bool needsInputFileVisitation() override { return true; }
+ bool needsSystemInputFileVisitation() override { return true; }
+ bool visitInputFile(StringRef Filename, bool IsSystem,
+ bool IsOverridden) override;
+};
+}
+
+void ModuleDependencyCollector::attachToASTReader(ASTReader &R) {
+ R.addListener(new ModuleDependencyListener(*this));
+}
+
+std::error_code ModuleDependencyListener::copyToRoot(StringRef Src) {
+ using namespace llvm::sys;
+ SmallString<256> Dest = Collector.getDest();
+ SmallString<256> AbsoluteSrc = Src;
+ fs::make_absolute(AbsoluteSrc);
+ path::append(Dest, path::relative_path(AbsoluteSrc));
+ if (std::error_code EC = fs::create_directories(path::parent_path(Dest),
+ /*IgnoreExisting=*/true))
+ return EC;
+ if (std::error_code EC = fs::copy_file(AbsoluteSrc.str(), Dest.str()))
+ return EC;
+ return std::error_code();
+}
+
+bool ModuleDependencyListener::visitInputFile(StringRef Filename, bool IsSystem,
+ bool IsOverridden) {
+ if (!Collector.insertSeen(Filename))
+ return true;
+ if (copyToRoot(Filename))
+ Collector.setHasErrors();
+ return true;
+}
--
1.9.3 (Apple Git-50)
>From e3b8d76aa70b136b297f71c4d7ae48494527b1f2 Mon Sep 17 00:00:00 2001
From: Justin Bogner <[email protected]>
Date: Mon, 9 Jun 2014 16:00:56 -0700
Subject: [PATCH 2/2] Frontend: Include a VFS map when dumping module
dependencies
Create a mapping from the module dependency dump directory back to the
original locations of the dumped files suitable for use with
-ivfsoverlay when trying to reproduce a build using the module
dependencies.
This currently encodes the absolute path to the dump directory, due to
limitations in the VFS system. Until we can handle relative paths in
the VFS, users of the VFS map may need to run a simple search and
replace in the file.
---
include/clang/Frontend/Utils.h | 7 +++++++
lib/Frontend/ModuleDependencyCollector.cpp | 17 +++++++++++++++++
2 files changed, 24 insertions(+)
diff --git a/include/clang/Frontend/Utils.h b/include/clang/Frontend/Utils.h
index f793e7c..a791cd5 100644
--- a/include/clang/Frontend/Utils.h
+++ b/include/clang/Frontend/Utils.h
@@ -15,6 +15,7 @@
#define LLVM_CLANG_FRONTEND_UTILS_H
#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/VirtualFileSystem.h"
#include "llvm/ADT/IntrusiveRefCntPtr.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSet.h"
@@ -87,16 +88,22 @@ class ModuleDependencyCollector {
std::string DestDir;
bool HasErrors;
llvm::StringSet<> Seen;
+ vfs::YAMLVFSWriter VFSWriter;
public:
StringRef getDest() { return DestDir; }
bool insertSeen(StringRef Filename) { return Seen.insert(Filename); }
void setHasErrors() { HasErrors = true; }
+ void addFileMapping(StringRef VPath, StringRef RPath) {
+ VFSWriter.addFileMapping(VPath, RPath);
+ }
void attachToASTReader(ASTReader &R);
+ void writeFileMap();
bool hasErrors() { return HasErrors; }
ModuleDependencyCollector(std::string DestDir)
: DestDir(DestDir), HasErrors(false) {}
+ ~ModuleDependencyCollector() { writeFileMap(); }
};
/// AttachDependencyGraphGen - Create a dependency graph generator, and attach
diff --git a/lib/Frontend/ModuleDependencyCollector.cpp b/lib/Frontend/ModuleDependencyCollector.cpp
index 0d2bfc1..d26f2ae 100644
--- a/lib/Frontend/ModuleDependencyCollector.cpp
+++ b/lib/Frontend/ModuleDependencyCollector.cpp
@@ -40,6 +40,22 @@ void ModuleDependencyCollector::attachToASTReader(ASTReader &R) {
R.addListener(new ModuleDependencyListener(*this));
}
+void ModuleDependencyCollector::writeFileMap() {
+ if (Seen.empty())
+ return;
+
+ SmallString<256> Dest = getDest();
+ llvm::sys::path::append(Dest, "vfs.yaml");
+
+ std::string ErrorInfo;
+ llvm::raw_fd_ostream OS(Dest.c_str(), ErrorInfo, llvm::sys::fs::F_Text);
+ if (!ErrorInfo.empty()) {
+ setHasErrors();
+ return;
+ }
+ VFSWriter.write(OS);
+}
+
std::error_code ModuleDependencyListener::copyToRoot(StringRef Src) {
using namespace llvm::sys;
SmallString<256> Dest = Collector.getDest();
@@ -51,6 +67,7 @@ std::error_code ModuleDependencyListener::copyToRoot(StringRef Src) {
return EC;
if (std::error_code EC = fs::copy_file(AbsoluteSrc.str(), Dest.str()))
return EC;
+ Collector.addFileMapping(AbsoluteSrc.str(), Dest.str());
return std::error_code();
}
--
1.9.3 (Apple Git-50)
>From 93adecfe0e1742c7dbebefd12dfc6c2426f391f6 Mon Sep 17 00:00:00 2001
From: Justin Bogner <[email protected]>
Date: Mon, 12 May 2014 23:39:04 -0700
Subject: [PATCH] Support: Add llvm::sys::fs::copy_file
A function to copy one file's contents to another.
---
include/llvm/Support/FileSystem.h | 6 ++++++
lib/Support/Path.cpp | 29 +++++++++++++++++++++++++++++
2 files changed, 35 insertions(+)
diff --git a/include/llvm/Support/FileSystem.h b/include/llvm/Support/FileSystem.h
index 6abe904..d8539bf 100644
--- a/include/llvm/Support/FileSystem.h
+++ b/include/llvm/Support/FileSystem.h
@@ -335,6 +335,12 @@ std::error_code remove(const Twine &path, bool IgnoreNonExisting = true);
/// @param to The path to rename to. This is created.
std::error_code rename(const Twine &from, const Twine &to);
+/// @brief Copy the contents of \a From to \a To.
+///
+/// @param From The path to copy from.
+/// @param To The path to copy to. This is created.
+error_code copy_file(const Twine &From, const Twine &To);
+
/// @brief Resize path to size. File is resized as if by POSIX truncate().
///
/// @param path Input path.
diff --git a/lib/Support/Path.cpp b/lib/Support/Path.cpp
index 15edf0dd..3578ab6 100644
--- a/lib/Support/Path.cpp
+++ b/lib/Support/Path.cpp
@@ -846,6 +846,35 @@ std::error_code create_directories(const Twine &Path, bool IgnoreExisting) {
return create_directory(P, IgnoreExisting);
}
+error_code copy_file(const Twine &From, const Twine &To) {
+ int ReadFD, WriteFD;
+ if (error_code EC = openFileForRead(From, ReadFD))
+ return EC;
+ if (error_code EC = openFileForWrite(To, WriteFD, F_None)) {
+ close(ReadFD);
+ return EC;
+ }
+
+ const size_t BufSize = 4096;
+ char *Buf = new char[BufSize];
+ int Bytes;
+ for (;;) {
+ Bytes = read(ReadFD, Buf, BufSize);
+ if (Bytes <= 0)
+ break;
+ Bytes = write(WriteFD, Buf, Bytes);
+ if (Bytes <= 0)
+ break;
+ }
+ close(ReadFD);
+ close(WriteFD);
+ delete[] Buf;
+
+ if (Bytes < 0)
+ return error_code(errno, posix_category());
+ return error_code();
+}
+
bool exists(file_status status) {
return status_known(status) && status.type() != file_type::file_not_found;
}
--
1.9.3 (Apple Git-50)
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
