Ben Langmuir <[email protected]> writes: > On Jun 19, 2014, at 10:31 AM, Justin Bogner <[email protected]> wrote: > > Ben Langmuir <[email protected]> writes: > > Hi Justin, > > Thanks for working on this! It’s looking pretty close. > > +/// Append the absolute path in Nested to the path given by Root. > This will > +/// remove directory traversal from the resulting nested path. > +static void appendNestedPath(SmallVectorImpl<char> &Root, > + SmallVectorImpl<char> &Nested) { > + using namespace llvm::sys; > + SmallVector<StringRef, 16> ComponentStack; > + > + StringRef Rel = path::relative_path(StringRef(Nested.begin(), > Nested.size())); > > You seem to have manually inlined Nested.str() ;-) Maybe just make > your Nested parameter a StringRef? > > Right, not sure what I was thinking there :). I'll pass in a StringRef > instead. > > + // We need an absolute path to append to the root. > + SmallString<256> AbsoluteSrc = Src; > + fs::make_absolute(AbsoluteSrc); > + // Build the destination path. > + SmallString<256> Dest = Collector.getDest(); > + size_t RootLen = Dest.size(); > + appendNestedPath(Dest, AbsoluteSrc); > > Do we need to escape this somehow on Windows, since you might get C: > in the middle of your path? > > And in general, will this work if Dest ends with a path separator? > Then you would end up with // in the middle, which could potentially > be eaten at some point (not sure). > > The call to path::relative_path in appendNestedPath takes care of both > of these issues. It's strips off root_name (ie, C:) and root_directory > (ie, /). > > It sure does, silly me. > > +bool ModuleDependencyListener::visitInputFile(StringRef Filename, > bool IsSystem, > + bool IsOverridden) > { > + if (!Collector.insertSeen(Filename)) > + return true; > + if (copyToRoot(Filename)) > + Collector.setHasErrors(); > + return true; > +} > > This is clearer to me if you invert the first if, but you decide. > if (Collector.insertSeen(Filename)) > if (copyToRoot(Filename)) > Collector.setHasErrors(); > return true; > > Sure, I'm happy with either. > > +// RUN: find %t/vfs -type f | FileCheck %s -check-prefix=DUMP > +// DUMP: AlsoDependsOnModule.framework/Headers/ > AlsoDependsOnModule.h > +// DUMP: > Module.framework/Frameworks/SubFramework.framework/Headers/ > SubFramework.h > > REQUIRES: shell, since you need ‘find’. This applies to both tests. > Also you won’t get the path separators you expect on Windows. > > Hmm. Is there a good way to check if the files are created without find? > Assuming there is, I'll change it use regex for the path separators, as > I think the extra platform coverage here is worthwhile. > > I can’t think of a cleaner way to do this. > > This isn’t really the place to discuss llvm patches, but... > > + char *Buf = new char[BufSize]; > > If you don’t want to put 4 KB on the stack, how about std::vector with > its data() method? > > Yeah, 4k seemed like a bit much for the stack (though, this is always a > leaf call, so maybe it's fine). > > Is a vector really better here? Since I have to manually manage closing > the files anyway, the new/delete doesn't feel out of place, and using a > std::vector or a std::unique_ptr purely as an RAII container muddies up > what this is doing a bit. > > I don’t feel strongly about it, so go with what you have. > > + for (;;) { > + Bytes = read(ReadFD, Buf, BufSize); > + if (Bytes <= 0) > + break; > + Bytes = write(WriteFD, Buf, Bytes); > + if (Bytes <= 0) > + break; > + } > > This doesn’t seem sufficiently paranoid about the number of bytes > actually written by ‘write’. > > Right. This should probably loop on the write as well. I'll update that.
New patches attached.
>From 6cb019281647ec36fecb8e82eeb6e14f04ec59c1 Mon Sep 17 00:00:00 2001 From: Justin Bogner <[email protected]> Date: Mon, 12 May 2014 23:16:57 -0700 Subject: [PATCH] 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 using -ivfsoverlay with the generated vfs.yaml. 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. We currently encode 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/Driver/Options.td | 2 + include/clang/Frontend/CompilerInstance.h | 7 ++ include/clang/Frontend/DependencyOutputOptions.h | 5 +- include/clang/Frontend/Utils.h | 26 ++++++ lib/Frontend/CMakeLists.txt | 1 + lib/Frontend/CompilerInstance.cpp | 22 +++++ lib/Frontend/CompilerInvocation.cpp | 2 + lib/Frontend/ModuleDependencyCollector.cpp | 109 +++++++++++++++++++++++ test/Modules/dependency-dump-dependent-module.m | 27 ++++++ test/Modules/dependency-dump.m | 25 ++++++ 10 files changed, 225 insertions(+), 1 deletion(-) create mode 100644 lib/Frontend/ModuleDependencyCollector.cpp create mode 100644 test/Modules/dependency-dump-dependent-module.m create mode 100644 test/Modules/dependency-dump.m 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..a791cd5 100644 --- a/include/clang/Frontend/Utils.h +++ b/include/clang/Frontend/Utils.h @@ -15,8 +15,10 @@ #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" #include "llvm/Option/OptSpecifier.h" namespace llvm { @@ -80,6 +82,30 @@ 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; + 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 /// 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..03a2c22 100644 --- a/lib/Frontend/CompilerInstance.cpp +++ b/lib/Frontend/CompilerInstance.cpp @@ -116,6 +116,16 @@ void CompilerInstance::setModuleManager(IntrusiveRefCntPtr<ASTReader> Reader) { ModuleManager = Reader; } +std::shared_ptr<ModuleDependencyCollector> +CompilerInstance::getModuleDepCollector() const { + return ModuleDepCollector; +} + +void CompilerInstance::setModuleDepCollector( + std::shared_ptr<ModuleDependencyCollector> Collector) { + ModuleDepCollector = Collector; +} + // Diagnostics static void SetUpDiagnosticLog(DiagnosticOptions *DiagOpts, const CodeGenOptions *CodeGenOpts, @@ -278,6 +288,11 @@ void CompilerInstance::createPreprocessor(TranslationUnitKind TUKind) { AttachDependencyGraphGen(*PP, DepOpts.DOTOutputFile, getHeaderSearchOpts().Sysroot); + // 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. + if (!ModuleDepCollector && !DepOpts.ModuleDependencyOutputDir.empty()) + ModuleDepCollector = std::make_shared<ModuleDependencyCollector>( + DepOpts.ModuleDependencyOutputDir); // Handle generating header include information, if requested. if (DepOpts.ShowHeaderIncludes) @@ -851,6 +866,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 +1230,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..8e4402b --- /dev/null +++ b/lib/Frontend/ModuleDependencyCollector.cpp @@ -0,0 +1,109 @@ +//===--- 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/iterator_range.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)); +} + +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); +} + +/// Append the absolute path in Nested to the path given by Root. This will +/// remove directory traversal from the resulting nested path. +static void appendNestedPath(SmallVectorImpl<char> &Root, StringRef Nested) { + using namespace llvm::sys; + SmallVector<StringRef, 16> ComponentStack; + + StringRef Rel = path::relative_path(Nested); + for (StringRef C : llvm::make_range(path::begin(Rel), path::end(Rel))) { + if (C == ".") + continue; + if (C == "..") { + assert(ComponentStack.size() && "Path traverses out of parent"); + ComponentStack.pop_back(); + } else + ComponentStack.push_back(C); + } + // The stack is now the path without any directory traversal. + for (StringRef C : ComponentStack) + path::append(Root, C); +} + +std::error_code ModuleDependencyListener::copyToRoot(StringRef Src) { + using namespace llvm::sys; + + // We need an absolute path to append to the root. + SmallString<256> AbsoluteSrc = Src; + fs::make_absolute(AbsoluteSrc); + // Build the destination path. + SmallString<256> Dest = Collector.getDest(); + size_t RootLen = Dest.size(); + appendNestedPath(Dest, AbsoluteSrc); + + // Copy the file into place. + 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; + // Use the absolute path under the root for the file mapping. + Collector.addFileMapping(Dest.substr(RootLen), Dest.str()); + return std::error_code(); +} + +bool ModuleDependencyListener::visitInputFile(StringRef Filename, bool IsSystem, + bool IsOverridden) { + if (Collector.insertSeen(Filename)) + if (copyToRoot(Filename)) + Collector.setHasErrors(); + return true; +} diff --git a/test/Modules/dependency-dump-dependent-module.m b/test/Modules/dependency-dump-dependent-module.m new file mode 100644 index 0000000..5308f7e --- /dev/null +++ b/test/Modules/dependency-dump-dependent-module.m @@ -0,0 +1,27 @@ +// When a module depends on another, check that we dump the dependency header +// files for both. + +// RUN: rm -rf %t +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -module-dependency-dir %t/vfs -F %S/Inputs -I %S/Inputs -verify %s +// expected-no-diagnostics + +// RUN: FileCheck %s -check-prefix=VFS < %t/vfs/vfs.yaml +// VFS: 'name': "AlsoDependsOnModule.h" +// VFS: 'name': "SubFramework.h" +// VFS: 'name': "Treasure.h" +// VFS: 'name': "Module.h" +// VFS: 'name': "Sub.h" +// VFS: 'name': "Sub2.h" + +// TODO: We need shell to use find here. Is there a simpler way? +// REQUIRES: shell + +// RUN: find %t/vfs -type f | FileCheck %s -check-prefix=DUMP +// DUMP: AlsoDependsOnModule.framework/Headers/AlsoDependsOnModule.h +// DUMP: Module.framework/Frameworks/SubFramework.framework/Headers/SubFramework.h +// DUMP: Module.framework/Headers/Buried/Treasure.h +// DUMP: Module.framework/Headers/Module.h +// DUMP: Module.framework/Headers/Sub.h +// DUMP: Module.framework/Headers/Sub2.h + +@import AlsoDependsOnModule; diff --git a/test/Modules/dependency-dump.m b/test/Modules/dependency-dump.m new file mode 100644 index 0000000..58d6c15 --- /dev/null +++ b/test/Modules/dependency-dump.m @@ -0,0 +1,25 @@ +// Check that we can dump all of the headers a module depends on, and a VFS map +// for the same. + +// RUN: rm -rf %t +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -module-dependency-dir %t/vfs -F %S/Inputs -I %S/Inputs -verify %s +// expected-no-diagnostics + +// RUN: FileCheck %s -check-prefix=VFS -input-file %t/vfs/vfs.yaml +// VFS: 'name': "SubFramework.h" +// VFS: 'name': "Treasure.h" +// VFS: 'name': "Module.h" +// VFS: 'name': "Sub.h" +// VFS: 'name': "Sub2.h" + +// TODO: We need shell to use find here. Is there a simpler way? +// REQUIRES: shell + +// RUN: find %t/vfs -type f | FileCheck %s -check-prefix=DUMP +// DUMP: Module.framework/Frameworks/SubFramework.framework/Headers/SubFramework.h +// DUMP: Module.framework/Headers/Buried/Treasure.h +// DUMP: Module.framework/Headers/Module.h +// DUMP: Module.framework/Headers/Sub.h +// DUMP: Module.framework/Headers/Sub2.h + +@import Module; -- 1.9.3 (Apple Git-50)
>From b85b07b3f314672876adf739e79ffdc49f5fa5ff 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 | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/include/llvm/Support/FileSystem.h b/include/llvm/Support/FileSystem.h index 6abe904..e56e2b7 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. +std::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..535394c 100644 --- a/lib/Support/Path.cpp +++ b/lib/Support/Path.cpp @@ -846,6 +846,40 @@ std::error_code create_directories(const Twine &Path, bool IgnoreExisting) { return create_directory(P, IgnoreExisting); } +std::error_code copy_file(const Twine &From, const Twine &To) { + int ReadFD, WriteFD; + if (std::error_code EC = openFileForRead(From, ReadFD)) + return EC; + if (std::error_code EC = openFileForWrite(To, WriteFD, F_None)) { + close(ReadFD); + return EC; + } + + const size_t BufSize = 4096; + char *Buf = new char[BufSize]; + int BytesRead = 0, BytesWritten = 0; + for (;;) { + BytesRead = read(ReadFD, Buf, BufSize); + if (BytesRead <= 0) + break; + while (BytesRead) { + BytesWritten = write(WriteFD, Buf, BytesRead); + if (BytesWritten < 0) + break; + BytesRead -= BytesWritten; + } + if (BytesWritten < 0) + break; + } + close(ReadFD); + close(WriteFD); + delete[] Buf; + + if (BytesRead < 0 || BytesWritten < 0) + return std::error_code(errno, std::generic_category()); + return std::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
