From e7f38db2358bfe75220ef9e990f9b4cc443a36ed Mon Sep 17 00:00:00 2001 From: Robert Widmann Date: Thu, 28 May 2020 14:21:08 -0700 Subject: [PATCH 1/4] [NFC] Have Evaluator's Constructor Read LangOptions It's a touch cleaner to do this than passing around a pile of bools. --- include/swift/AST/Evaluator.h | 5 +---- lib/AST/ASTContext.cpp | 5 +---- lib/AST/Evaluator.cpp | 16 ++++++++-------- unittests/AST/ArithmeticEvaluator.cpp | 19 +++++++++++-------- 4 files changed, 21 insertions(+), 24 deletions(-) diff --git a/include/swift/AST/Evaluator.h b/include/swift/AST/Evaluator.h index d39c68ab1bcae..95730fffc812c 100644 --- a/include/swift/AST/Evaluator.h +++ b/include/swift/AST/Evaluator.h @@ -243,10 +243,7 @@ class Evaluator { public: /// Construct a new evaluator that can emit cyclic-dependency /// diagnostics through the given diagnostics engine. - Evaluator(DiagnosticEngine &diags, - bool debugDumpCycles, - bool buildDependencyGraph, - bool enableExperimentalPrivateDeps); + Evaluator(DiagnosticEngine &diags, const LangOptions &opts); /// Emit GraphViz output visualizing the request graph. void emitRequestEvaluatorGraphViz(llvm::StringRef graphVizPath); diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp index 8d6e5295cc654..d5a1ec4c45ae7 100644 --- a/lib/AST/ASTContext.cpp +++ b/lib/AST/ASTContext.cpp @@ -549,10 +549,7 @@ ASTContext::ASTContext(LangOptions &langOpts, TypeCheckerOptions &typeckOpts, : LangOpts(langOpts), TypeCheckerOpts(typeckOpts), SearchPathOpts(SearchPathOpts), SourceMgr(SourceMgr), Diags(Diags), - evaluator(Diags, - langOpts.DebugDumpCycles, - langOpts.BuildRequestDependencyGraph, - langOpts.EnableExperientalPrivateIntransitiveDependencies), + evaluator(Diags, langOpts), TheBuiltinModule(createBuiltinModule(*this)), StdlibModuleName(getIdentifier(STDLIB_NAME)), SwiftShimsModuleName(getIdentifier(SWIFT_SHIMS_NAME)), diff --git a/lib/AST/Evaluator.cpp b/lib/AST/Evaluator.cpp index 472a5387e0f5d..5d2b626db8d2f 100644 --- a/lib/AST/Evaluator.cpp +++ b/lib/AST/Evaluator.cpp @@ -16,6 +16,7 @@ //===----------------------------------------------------------------------===// #include "swift/AST/Evaluator.h" #include "swift/AST/DiagnosticEngine.h" +#include "swift/Basic/LangOptions.h" #include "swift/Basic/Range.h" #include "swift/Basic/SourceManager.h" #include "llvm/ADT/StringExtras.h" @@ -62,21 +63,20 @@ void Evaluator::registerRequestFunctions( } static evaluator::DependencyRecorder::Mode -computeDependencyModeFromFlags(bool enableExperimentalPrivateDeps) { +computeDependencyModeFromFlags(const LangOptions &opts) { using Mode = evaluator::DependencyRecorder::Mode; - if (enableExperimentalPrivateDeps) { + if (opts.EnableExperientalPrivateIntransitiveDependencies) { return Mode::ExperimentalPrivateDependencies; } return Mode::StatusQuo; } -Evaluator::Evaluator(DiagnosticEngine &diags, bool debugDumpCycles, - bool buildDependencyGraph, - bool enableExperimentalPrivateDeps) - : diags(diags), debugDumpCycles(debugDumpCycles), - buildDependencyGraph(buildDependencyGraph), - recorder{computeDependencyModeFromFlags(enableExperimentalPrivateDeps)} {} +Evaluator::Evaluator(DiagnosticEngine &diags, const LangOptions &opts) + : diags(diags), + debugDumpCycles(opts.DebugDumpCycles), + buildDependencyGraph(opts.BuildRequestDependencyGraph), + recorder{computeDependencyModeFromFlags(opts)} {} void Evaluator::emitRequestEvaluatorGraphViz(llvm::StringRef graphVizPath) { std::error_code error; diff --git a/unittests/AST/ArithmeticEvaluator.cpp b/unittests/AST/ArithmeticEvaluator.cpp index b41928fbf2feb..00c3a495d9a32 100644 --- a/unittests/AST/ArithmeticEvaluator.cpp +++ b/unittests/AST/ArithmeticEvaluator.cpp @@ -16,6 +16,7 @@ #include "swift/AST/DiagnosticEngine.h" #include "swift/AST/Evaluator.h" #include "swift/AST/SimpleRequest.h" +#include "swift/Basic/LangOptions.h" #include "swift/Basic/SourceManager.h" #include "gtest/gtest.h" #include @@ -218,10 +219,11 @@ TEST(ArithmeticEvaluator, Simple) { SourceManager sourceMgr; DiagnosticEngine diags(sourceMgr); - Evaluator evaluator(diags, - /*debugDumpCycles=*/false, - /*buildDependencyGraph=*/true, - /*privateDependencies*/false); + LangOptions opts; + opts.DebugDumpCycles = false; + opts.BuildRequestDependencyGraph = true; + opts.EnableExperientalPrivateIntransitiveDependencies = false; + Evaluator evaluator(diags, opts); evaluator.registerRequestFunctions(Zone::ArithmeticEvaluator, arithmeticRequestFunctions); @@ -344,10 +346,11 @@ TEST(ArithmeticEvaluator, Cycle) { SourceManager sourceMgr; DiagnosticEngine diags(sourceMgr); - Evaluator evaluator(diags, - /*debugDumpCycles=*/false, - /*buildDependencyGraph=*/false, - /*privateDependencies*/false); + LangOptions opts; + opts.DebugDumpCycles = false; + opts.BuildRequestDependencyGraph = false; + opts.EnableExperientalPrivateIntransitiveDependencies = false; + Evaluator evaluator(diags, opts); evaluator.registerRequestFunctions(Zone::ArithmeticEvaluator, arithmeticRequestFunctions); From a4bcbfab48b814935b72a4d46ee2ce3344fd2a40 Mon Sep 17 00:00:00 2001 From: Robert Widmann Date: Tue, 2 Jun 2020 10:12:46 -0700 Subject: [PATCH 2/4] [NFC] Remove a Dead Parameter From Clang Module Loading The only caller consuming the data that resulted from this bit has it set to false. Additionally, the side effect of force-loading the overlays is already handled unconditionally by the call to namelookup::getAllImports. --- lib/ClangImporter/ClangImporter.cpp | 14 ++++---------- lib/ClangImporter/ImporterImpl.h | 3 +-- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp index 6b11ca1992bd9..48411fec7a4e6 100644 --- a/lib/ClangImporter/ClangImporter.cpp +++ b/lib/ClangImporter/ClangImporter.cpp @@ -1779,8 +1779,7 @@ ModuleDecl *ClangImporter::Implementation::loadModuleClang( if (!clangModule) return nullptr; - return finishLoadingClangModule(importLoc, clangModule, - /*preferOverlay=*/false); + return finishLoadingClangModule(importLoc, clangModule); } ModuleDecl * @@ -1800,7 +1799,7 @@ ModuleDecl *ClangImporter::Implementation::loadModule( } ModuleDecl *ClangImporter::Implementation::finishLoadingClangModule( - SourceLoc importLoc, const clang::Module *clangModule, bool findOverlay) { + SourceLoc importLoc, const clang::Module *clangModule) { assert(clangModule); // Bump the generation count. @@ -1842,17 +1841,13 @@ ModuleDecl *ClangImporter::Implementation::finishLoadingClangModule( } if (clangModule->isSubModule()) { - finishLoadingClangModule(importLoc, clangModule->getTopLevelModule(), true); + finishLoadingClangModule(importLoc, clangModule->getTopLevelModule()); } else { ModuleDecl *&loaded = SwiftContext.LoadedModules[result->getName()]; if (!loaded) loaded = result; } - if (findOverlay) - if (ModuleDecl *overlay = wrapperUnit->getOverlayModule()) - result = overlay; - return result; } @@ -1876,8 +1871,7 @@ void ClangImporter::Implementation::handleDeferredImports(SourceLoc diagLoc) { // officially supported with bridging headers: app targets and unit tests // only. Unfortunately that's not enforced. for (size_t i = 0; i < ImportedHeaderExports.size(); ++i) { - (void)finishLoadingClangModule(diagLoc, ImportedHeaderExports[i], - /*preferOverlay=*/true); + (void)finishLoadingClangModule(diagLoc, ImportedHeaderExports[i]); } } diff --git a/lib/ClangImporter/ImporterImpl.h b/lib/ClangImporter/ImporterImpl.h index 7963151dda780..67127fc15db6c 100644 --- a/lib/ClangImporter/ImporterImpl.h +++ b/lib/ClangImporter/ImporterImpl.h @@ -926,8 +926,7 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation /// Constructs a Swift module for the given Clang module. ModuleDecl *finishLoadingClangModule(SourceLoc importLoc, - const clang::Module *clangModule, - bool preferOverlay); + const clang::Module *clangModule); /// Call finishLoadingClangModule on each deferred import collected /// while scanning a bridging header or PCH. From c8077e8974a538356c695cb7d93df485530d050d Mon Sep 17 00:00:00 2001 From: Robert Widmann Date: Tue, 2 Jun 2020 11:00:40 -0700 Subject: [PATCH 3/4] [Gardening] Add a SourceLoc For Cross-Import Overlay Diagnostics --- lib/ClangImporter/ClangImporter.cpp | 12 ++++++------ lib/ClangImporter/ImporterImpl.h | 7 ++++--- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp index 48411fec7a4e6..0ca7686f31fb2 100644 --- a/lib/ClangImporter/ClangImporter.cpp +++ b/lib/ClangImporter/ClangImporter.cpp @@ -1779,7 +1779,7 @@ ModuleDecl *ClangImporter::Implementation::loadModuleClang( if (!clangModule) return nullptr; - return finishLoadingClangModule(importLoc, clangModule); + return finishLoadingClangModule(clangModule, importLoc); } ModuleDecl * @@ -1799,7 +1799,7 @@ ModuleDecl *ClangImporter::Implementation::loadModule( } ModuleDecl *ClangImporter::Implementation::finishLoadingClangModule( - SourceLoc importLoc, const clang::Module *clangModule) { + const clang::Module *clangModule, SourceLoc importLoc) { assert(clangModule); // Bump the generation count. @@ -1841,7 +1841,7 @@ ModuleDecl *ClangImporter::Implementation::finishLoadingClangModule( } if (clangModule->isSubModule()) { - finishLoadingClangModule(importLoc, clangModule->getTopLevelModule()); + finishLoadingClangModule(clangModule->getTopLevelModule(), importLoc); } else { ModuleDecl *&loaded = SwiftContext.LoadedModules[result->getName()]; if (!loaded) @@ -1871,7 +1871,7 @@ void ClangImporter::Implementation::handleDeferredImports(SourceLoc diagLoc) { // officially supported with bridging headers: app targets and unit tests // only. Unfortunately that's not enforced. for (size_t i = 0; i < ImportedHeaderExports.size(); ++i) { - (void)finishLoadingClangModule(diagLoc, ImportedHeaderExports[i]); + (void)finishLoadingClangModule(ImportedHeaderExports[i], diagLoc); } } @@ -2012,7 +2012,7 @@ ClangImporter::Implementation::~Implementation() { } ClangModuleUnit *ClangImporter::Implementation::getWrapperForModule( - const clang::Module *underlying) { + const clang::Module *underlying, SourceLoc diagLoc) { auto &cacheEntry = ModuleWrappers[underlying]; if (ClangModuleUnit *cached = cacheEntry.getPointer()) return cached; @@ -2027,7 +2027,7 @@ ClangModuleUnit *ClangImporter::Implementation::getWrapperForModule( auto file = new (SwiftContext) ClangModuleUnit(*wrapper, *this, underlying); wrapper->addFile(*file); - SwiftContext.getClangModuleLoader()->findOverlayFiles(SourceLoc(), wrapper, file); + SwiftContext.getClangModuleLoader()->findOverlayFiles(diagLoc, wrapper, file); cacheEntry.setPointer(file); return file; diff --git a/lib/ClangImporter/ImporterImpl.h b/lib/ClangImporter/ImporterImpl.h index 67127fc15db6c..3a52658176ba2 100644 --- a/lib/ClangImporter/ImporterImpl.h +++ b/lib/ClangImporter/ImporterImpl.h @@ -922,11 +922,12 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation /// Retrieves the Swift wrapper for the given Clang module, creating /// it if necessary. - ClangModuleUnit *getWrapperForModule(const clang::Module *underlying); + ClangModuleUnit *getWrapperForModule(const clang::Module *underlying, + SourceLoc importLoc = SourceLoc()); /// Constructs a Swift module for the given Clang module. - ModuleDecl *finishLoadingClangModule(SourceLoc importLoc, - const clang::Module *clangModule); + ModuleDecl *finishLoadingClangModule(const clang::Module *clangModule, + SourceLoc importLoc); /// Call finishLoadingClangModule on each deferred import collected /// while scanning a bridging header or PCH. From d2e1336af72c33f54d55391449fdcd31cc3bd994 Mon Sep 17 00:00:00 2001 From: Robert Widmann Date: Tue, 2 Jun 2020 11:19:11 -0700 Subject: [PATCH 4/4] [NFC] Use getWrapperForModule to Simplify Clang Module Loading --- lib/ClangImporter/ClangImporter.cpp | 39 ++++++----------------------- 1 file changed, 7 insertions(+), 32 deletions(-) diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp index 0ca7686f31fb2..be31c41d0f621 100644 --- a/lib/ClangImporter/ClangImporter.cpp +++ b/lib/ClangImporter/ClangImporter.cpp @@ -1805,38 +1805,13 @@ ModuleDecl *ClangImporter::Implementation::finishLoadingClangModule( // Bump the generation count. bumpGeneration(); - auto &cacheEntry = ModuleWrappers[clangModule]; - ModuleDecl *result; - ClangModuleUnit *wrapperUnit; - if ((wrapperUnit = cacheEntry.getPointer())) { - result = wrapperUnit->getParentModule(); - if (!cacheEntry.getInt()) { - // Force load overlays for all imported modules. - // FIXME: This forces the creation of wrapper modules for all imports as - // well, and may do unnecessary work. - cacheEntry.setInt(true); - (void) namelookup::getAllImports(result); - } - } else { - // Build the representation of the Clang module in Swift. - // FIXME: The name of this module could end up as a key in the ASTContext, - // but that's not correct for submodules. - Identifier name = SwiftContext.getIdentifier((*clangModule).Name); - result = ModuleDecl::create(name, SwiftContext); - result->setIsSystemModule(clangModule->IsSystem); - result->setIsNonSwiftModule(); - result->setHasResolvedImports(); - - wrapperUnit = - new (SwiftContext) ClangModuleUnit(*result, *this, clangModule); - result->addFile(*wrapperUnit); - SwiftContext.getClangModuleLoader() - ->findOverlayFiles(importLoc, result, wrapperUnit); - cacheEntry.setPointerAndInt(wrapperUnit, true); - - // Force load overlays for all imported modules. - // FIXME: This forces the creation of wrapper modules for all imports as - // well, and may do unnecessary work. + // Force load overlays for all imported modules. + // FIXME: This forces the creation of wrapper modules for all imports as + // well, and may do unnecessary work. + ClangModuleUnit *wrapperUnit = getWrapperForModule(clangModule, importLoc); + ModuleDecl *result = wrapperUnit->getParentModule(); + if (!ModuleWrappers[clangModule].getInt()) { + ModuleWrappers[clangModule].setInt(true); (void) namelookup::getAllImports(result); }