From 830ea5b7c75413526c19531f0180fa6e45b98919 Mon Sep 17 00:00:00 2001 From: Douglas Gregor Date: Fri, 30 Nov 2012 18:38:50 +0000 Subject: [PATCH] When an error occurs while building a module on demand, provide "While building module 'Foo' imported from..." notes (the same we we provide "In file included from..." notes) in the diagnostic, so that we know how this module got included in the first place. This is part of . git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@169021 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/SourceManager.h | 31 ++++++++++++++++ include/clang/Frontend/DiagnosticRenderer.h | 12 +++++-- include/clang/Frontend/TextDiagnostic.h | 4 +++ include/clang/Lex/PreprocessorOptions.h | 9 +---- lib/Frontend/CompilerInstance.cpp | 39 +++++++++++++-------- lib/Frontend/DiagnosticRenderer.cpp | 33 ++++++++++++++++- lib/Frontend/TextDiagnostic.cpp | 11 ++++++ test/Modules/build-fail-notes.m | 12 +++++++ test/Modules/cycles.c | 7 ++-- test/Modules/epic-fail.m | 2 ++ 10 files changed, 132 insertions(+), 28 deletions(-) create mode 100644 test/Modules/build-fail-notes.m diff --git a/include/clang/Basic/SourceManager.h b/include/clang/Basic/SourceManager.h index 5717816ff9..bd31c79ba2 100644 --- a/include/clang/Basic/SourceManager.h +++ b/include/clang/Basic/SourceManager.h @@ -40,6 +40,7 @@ #include "clang/Basic/SourceLocation.h" #include "llvm/Support/Allocator.h" #include "llvm/Support/DataTypes.h" +#include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/PointerIntPair.h" #include "llvm/ADT/PointerUnion.h" #include "llvm/ADT/IntrusiveRefCntPtr.h" @@ -508,6 +509,11 @@ public: }; +/// \brief The path used when building modules on demand, which is used +/// to provide a link between the source managers of the different compiler +/// instances. +typedef llvm::ArrayRef > ModuleBuildPath; + /// \brief This class handles loading and caching of source files into memory. /// /// This object owns the MemoryBuffer objects for all of the loaded @@ -645,6 +651,15 @@ class SourceManager : public RefCountedBase { mutable llvm::DenseMap MacroArgsCacheMap; + /// \brief The path of modules being built, which is used to detect + /// cycles in the module dependency graph as modules are being built, as + /// well as to describe + /// + /// There is no way to set this value from the command line. If we ever need + /// to do so (e.g., if on-demand module construction moves out-of-process), + /// we can add a cc1-level option to do so. + SmallVector, 2> StoredModuleBuildPath; + // SourceManager doesn't support copy construction. explicit SourceManager(const SourceManager&) LLVM_DELETED_FUNCTION; void operator=(const SourceManager&) LLVM_DELETED_FUNCTION; @@ -669,6 +684,22 @@ public: /// (likely to change while trying to use them). bool userFilesAreVolatile() const { return UserFilesAreVolatile; } + /// \brief Retrieve the module build path. + ModuleBuildPath getModuleBuildPath() const { + return StoredModuleBuildPath; + } + + /// \brief Set the module build path. + void setModuleBuildPath(ModuleBuildPath path) { + StoredModuleBuildPath.clear(); + StoredModuleBuildPath.append(path.begin(), path.end()); + } + + /// \brief Append an entry to the module build path. + void appendModuleBuildPath(StringRef moduleName, FullSourceLoc importLoc) { + StoredModuleBuildPath.push_back(std::make_pair(moduleName.str(),importLoc)); + } + /// \brief Create the FileID for a memory buffer that will represent the /// FileID for the main source. /// diff --git a/include/clang/Frontend/DiagnosticRenderer.h b/include/clang/Frontend/DiagnosticRenderer.h index 086bb137d4..6ca8d89ec9 100644 --- a/include/clang/Frontend/DiagnosticRenderer.h +++ b/include/clang/Frontend/DiagnosticRenderer.h @@ -92,7 +92,10 @@ protected: virtual void emitIncludeLocation(SourceLocation Loc, PresumedLoc PLoc, const SourceManager &SM) = 0; - + virtual void emitBuildingModuleLocation(SourceLocation Loc, PresumedLoc PLoc, + StringRef ModuleName, + const SourceManager &SM) = 0; + virtual void beginDiagnostic(DiagOrStoredDiag D, DiagnosticsEngine::Level Level) {} virtual void endDiagnostic(DiagOrStoredDiag D, @@ -103,6 +106,7 @@ private: void emitIncludeStack(SourceLocation Loc, DiagnosticsEngine::Level Level, const SourceManager &SM); void emitIncludeStackRecursively(SourceLocation Loc, const SourceManager &SM); + void emitModuleBuildPath(const SourceManager &SM); void emitMacroExpansionsAndCarets(SourceLocation Loc, DiagnosticsEngine::Level Level, SmallVectorImpl& Ranges, @@ -149,7 +153,11 @@ public: virtual void emitIncludeLocation(SourceLocation Loc, PresumedLoc PLoc, const SourceManager &SM); - + + virtual void emitBuildingModuleLocation(SourceLocation Loc, PresumedLoc PLoc, + StringRef ModuleName, + const SourceManager &SM); + virtual void emitNote(SourceLocation Loc, StringRef Message, const SourceManager *SM) = 0; }; diff --git a/include/clang/Frontend/TextDiagnostic.h b/include/clang/Frontend/TextDiagnostic.h index 51f841ddd3..859bec2cfe 100644 --- a/include/clang/Frontend/TextDiagnostic.h +++ b/include/clang/Frontend/TextDiagnostic.h @@ -103,6 +103,10 @@ protected: virtual void emitIncludeLocation(SourceLocation Loc, PresumedLoc PLoc, const SourceManager &SM); + virtual void emitBuildingModuleLocation(SourceLocation Loc, PresumedLoc PLoc, + StringRef ModuleName, + const SourceManager &SM); + private: void emitSnippetAndCaret(SourceLocation Loc, DiagnosticsEngine::Level Level, SmallVectorImpl& Ranges, diff --git a/include/clang/Lex/PreprocessorOptions.h b/include/clang/Lex/PreprocessorOptions.h index 857161ddc0..a670267f02 100644 --- a/include/clang/Lex/PreprocessorOptions.h +++ b/include/clang/Lex/PreprocessorOptions.h @@ -10,6 +10,7 @@ #ifndef LLVM_CLANG_LEX_PREPROCESSOROPTIONS_H_ #define LLVM_CLANG_LEX_PREPROCESSOROPTIONS_H_ +#include "clang/Basic/SourceLocation.h" #include "llvm/ADT/IntrusiveRefCntPtr.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" @@ -120,14 +121,6 @@ public: /// with support for lifetime-qualified pointers. ObjCXXARCStandardLibraryKind ObjCXXARCStandardLibrary; - /// \brief The path of modules being build, which is used to detect - /// cycles in the module dependency graph as modules are being built. - /// - /// There is no way to set this value from the command line. If we ever need - /// to do so (e.g., if on-demand module construction moves out-of-process), - /// we can add a cc1-level option to do so. - SmallVector ModuleBuildPath; - /// \brief Records the set of modules class FailedModulesSet : public llvm::RefCountedBase { llvm::StringSet<> Failed; diff --git a/lib/Frontend/CompilerInstance.cpp b/lib/Frontend/CompilerInstance.cpp index f87a635420..150f99578c 100644 --- a/lib/Frontend/CompilerInstance.cpp +++ b/lib/Frontend/CompilerInstance.cpp @@ -762,6 +762,7 @@ static void doCompileMapModule(void *UserData) { /// \brief Compile a module file for the given module, using the options /// provided by the importing compiler instance. static void compileModule(CompilerInstance &ImportingInstance, + SourceLocation ImportLoc, Module *Module, StringRef ModuleFileName) { llvm::LockFileManager Locked(ModuleFileName); @@ -797,10 +798,6 @@ static void compileModule(CompilerInstance &ImportingInstance, // Note the name of the module we're building. Invocation->getLangOpts()->CurrentModule = Module->getTopLevelModuleName(); - // Note that this module is part of the module build path, so that we - // can detect cycles in the module graph. - PPOpts.ModuleBuildPath.push_back(Module->getTopLevelModuleName()); - // Make sure that the failed-module structure has been allocated in // the importing instance, and propagate the pointer to the newly-created // instance. @@ -861,7 +858,18 @@ static void compileModule(CompilerInstance &ImportingInstance, &ImportingInstance.getDiagnosticClient(), /*ShouldOwnClient=*/true, /*ShouldCloneClient=*/true); - + + // Note that this module is part of the module build path, so that we + // can detect cycles in the module graph. + Instance.createFileManager(); // FIXME: Adopt file manager from importer? + Instance.createSourceManager(Instance.getFileManager()); + SourceManager &SourceMgr = Instance.getSourceManager(); + SourceMgr.setModuleBuildPath( + ImportingInstance.getSourceManager().getModuleBuildPath()); + SourceMgr.appendModuleBuildPath(Module->getTopLevelModuleName(), + FullSourceLoc(ImportLoc, ImportingInstance.getSourceManager())); + + // Construct a module-generating action. GenerateModuleAction CreateModuleAction; @@ -939,14 +947,17 @@ CompilerInstance::loadModule(SourceLocation ImportLoc, // build the module. // Check whether there is a cycle in the module graph. - SmallVectorImpl &ModuleBuildPath - = getPreprocessorOpts().ModuleBuildPath; - SmallVectorImpl::iterator Pos - = std::find(ModuleBuildPath.begin(), ModuleBuildPath.end(), ModuleName); - if (Pos != ModuleBuildPath.end()) { + ModuleBuildPath Path = getSourceManager().getModuleBuildPath(); + ModuleBuildPath::iterator Pos = Path.begin(), PosEnd = Path.end(); + for (; Pos != PosEnd; ++Pos) { + if (Pos->first == ModuleName) + break; + } + + if (Pos != PosEnd) { SmallString<256> CyclePath; - for (; Pos != ModuleBuildPath.end(); ++Pos) { - CyclePath += *Pos; + for (; Pos != PosEnd; ++Pos) { + CyclePath += Pos->first; CyclePath += " -> "; } CyclePath += ModuleName; @@ -970,7 +981,7 @@ CompilerInstance::loadModule(SourceLocation ImportLoc, getDiagnostics().Report(ModuleNameLoc, diag::warn_module_build) << ModuleName; BuildingModule = true; - compileModule(*this, Module, ModuleFileName); + compileModule(*this, ModuleNameLoc, Module, ModuleFileName); ModuleFile = FileMgr->getFile(ModuleFileName); if (!ModuleFile) @@ -1040,7 +1051,7 @@ CompilerInstance::loadModule(SourceLocation ImportLoc, return ModuleLoadResult(); } - compileModule(*this, Module, ModuleFileName); + compileModule(*this, ModuleNameLoc, Module, ModuleFileName); // Try loading the module again. ModuleFile = FileMgr->getFile(ModuleFileName); diff --git a/lib/Frontend/DiagnosticRenderer.cpp b/lib/Frontend/DiagnosticRenderer.cpp index 3143cc7b32..3599df82c7 100644 --- a/lib/Frontend/DiagnosticRenderer.cpp +++ b/lib/Frontend/DiagnosticRenderer.cpp @@ -205,8 +205,10 @@ void DiagnosticRenderer::emitIncludeStack(SourceLocation Loc, /// on the way back down. void DiagnosticRenderer::emitIncludeStackRecursively(SourceLocation Loc, const SourceManager &SM) { - if (Loc.isInvalid()) + if (Loc.isInvalid()) { + emitModuleBuildPath(SM); return; + } PresumedLoc PLoc = SM.getPresumedLoc(Loc, DiagOpts->ShowPresumedLoc); if (PLoc.isInvalid()) @@ -219,6 +221,21 @@ void DiagnosticRenderer::emitIncludeStackRecursively(SourceLocation Loc, emitIncludeLocation(Loc, PLoc, SM); } +/// \brief Emit the module build path, for cases where a module is (re-)built +/// on demand. +void DiagnosticRenderer::emitModuleBuildPath(const SourceManager &SM) { + ModuleBuildPath Path = SM.getModuleBuildPath(); + for (unsigned I = 0, N = Path.size(); I != N; ++I) { + const SourceManager &CurSM = Path[I].second.getManager(); + SourceLocation CurLoc = Path[I].second; + emitBuildingModuleLocation(CurLoc, + CurSM.getPresumedLoc(CurLoc, + DiagOpts->ShowPresumedLoc), + Path[I].first, + CurSM); + } +} + // Helper function to fix up source ranges. It takes in an array of ranges, // and outputs an array of ranges where we want to draw the range highlighting // around the location specified by CaretLoc. @@ -390,6 +407,20 @@ void DiagnosticNoteRenderer::emitIncludeLocation(SourceLocation Loc, emitNote(Loc, Message.str(), &SM); } +void +DiagnosticNoteRenderer::emitBuildingModuleLocation(SourceLocation Loc, + PresumedLoc PLoc, + StringRef ModuleName, + const SourceManager &SM) { + // Generate a note indicating the include location. + SmallString<200> MessageStorage; + llvm::raw_svector_ostream Message(MessageStorage); + Message << "while building module '" << ModuleName << "' imported from " + << PLoc.getFilename() << ':' << PLoc.getLine() << ":"; + emitNote(Loc, Message.str(), &SM); +} + + void DiagnosticNoteRenderer::emitBasicNote(StringRef Message) { emitNote(SourceLocation(), Message, 0); } diff --git a/lib/Frontend/TextDiagnostic.cpp b/lib/Frontend/TextDiagnostic.cpp index 35dabad606..65df875aea 100644 --- a/lib/Frontend/TextDiagnostic.cpp +++ b/lib/Frontend/TextDiagnostic.cpp @@ -884,6 +884,17 @@ void TextDiagnostic::emitIncludeLocation(SourceLocation Loc, OS << "In included file:\n"; } +void TextDiagnostic::emitBuildingModuleLocation(SourceLocation Loc, + PresumedLoc PLoc, + StringRef ModuleName, + const SourceManager &SM) { + if (DiagOpts->ShowLocation) + OS << "While building module '" << ModuleName << "' imported from " + << PLoc.getFilename() << ':' << PLoc.getLine() << ":\n"; + else + OS << "While building module '" << ModuleName << "':\n"; +} + /// \brief Emit a code snippet and caret line. /// /// This routine emits a single line's code snippet and caret line.. diff --git a/test/Modules/build-fail-notes.m b/test/Modules/build-fail-notes.m new file mode 100644 index 0000000000..fe5522342f --- /dev/null +++ b/test/Modules/build-fail-notes.m @@ -0,0 +1,12 @@ +// RUN: rm -rf %t +// RUN: %clang_cc1 -fmodule-cache-path %t -fmodules -F %S/Inputs -DgetModuleVersion="epic fail" %s 2>&1 | FileCheck %s + +@__experimental_modules_import DependsOnModule; + +// CHECK: While building module 'DependsOnModule' imported from +// CHECK: While building module 'Module' imported from +// CHECK: error: expected ';' after top level declarator +// CHECK: note: expanded from macro 'getModuleVersion' +// CHECK: fatal error: could not build module 'Module' +// CHECK: fatal error: could not build module 'DependsOnModule' +// CHECK-NOT: error: diff --git a/test/Modules/cycles.c b/test/Modules/cycles.c index 256f118cc0..5fcb41ec1f 100644 --- a/test/Modules/cycles.c +++ b/test/Modules/cycles.c @@ -3,10 +3,11 @@ // FIXME: When we have a syntax for modules in C, use that. @__experimental_modules_import MutuallyRecursive1; -// FIXME: Lots of redundant diagnostics here, because the preprocessor -// can't currently tell the parser not to try to load the module again. - +// CHECK: While building module 'MutuallyRecursive1' imported from +// CHECK: While building module 'MutuallyRecursive2' imported from // CHECK: MutuallyRecursive2.h:3:32: fatal error: cyclic dependency in module 'MutuallyRecursive1': MutuallyRecursive1 -> MutuallyRecursive2 -> MutuallyRecursive1 +// CHECK: While building module 'MutuallyRecursive1' imported from // CHECK: MutuallyRecursive1.h:2:32: fatal error: could not build module 'MutuallyRecursive2' // CHECK: cycles.c:4:32: fatal error: could not build module 'MutuallyRecursive1' +// CHECK-NOT: error: diff --git a/test/Modules/epic-fail.m b/test/Modules/epic-fail.m index 7d2cdcf602..32ac52d89a 100644 --- a/test/Modules/epic-fail.m +++ b/test/Modules/epic-fail.m @@ -4,8 +4,10 @@ @__experimental_modules_import Module; @__experimental_modules_import DependsOnModule; +// CHECK: While building module 'Module' imported from // CHECK: error: expected ';' after top level declarator // CHECK: note: expanded from macro 'getModuleVersion' // CHECK: fatal error: could not build module 'Module' +// CHECK: While building module 'DependsOnModule' imported from // CHECK: fatal error: could not build module 'Module' // CHECK-NOT: error: -- 2.40.0