From d8c823e159aed405d0e7371d8702aa4f910251d0 Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Fri, 23 Aug 2019 17:36:28 -0700 Subject: [PATCH 1/4] Module trace: explicitly ignore the main module I'm not sure why this wasn't already broken, but with the changes in the next commit the multifile module trace test fails complaining that it can't get a path for the main module. But we don't /need/ a path for the main module because it's not a dependency of itself. --- lib/FrontendTool/FrontendTool.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/FrontendTool/FrontendTool.cpp b/lib/FrontendTool/FrontendTool.cpp index d11fdd638a5..63b4d222b98 100644 --- a/lib/FrontendTool/FrontendTool.cpp +++ b/lib/FrontendTool/FrontendTool.cpp @@ -268,6 +268,8 @@ static bool emitLoadedModuleTraceIfNeeded(ModuleDecl *mainModule, for (auto &module : ctxt.LoadedModules) { ModuleDecl *loadedDecl = module.second; assert(loadedDecl && "Expected loaded module to be non-null."); + if (loadedDecl == mainModule) + continue; assert(!loadedDecl->getModuleFilename().empty() && "Don't know how to handle modules with empty names."); pathToModuleDecl.insert( From e479e1398da9b5196823ba5ce3e804e9bacc7c9c Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Fri, 23 Aug 2019 16:53:30 -0700 Subject: [PATCH 2/4] [Serialization] Use the module interface as the name of the file ...rather than the buffer, for a compiled module that came from a module interface. This was already happening at a higher level (ModuleDecl::getModuleFilename) so pushing it down to the low-level ModuleFile::getModuleFilename doesn't really change things much. The important fix that goes with this is that SerializedASTFile no longer leaks this name by storing it outside of ModuleFile. https://bugs.swift.org/browse/SR-11365 --- include/swift/AST/Module.h | 4 ---- include/swift/Serialization/ModuleFile.h | 7 +++++++ include/swift/Serialization/SerializedModuleLoader.h | 11 ----------- include/swift/Serialization/Validation.h | 3 --- lib/AST/Module.cpp | 4 ---- lib/Serialization/ModuleFile.cpp | 3 +-- lib/Serialization/SerializedModuleLoader.cpp | 1 - 7 files changed, 8 insertions(+), 25 deletions(-) diff --git a/include/swift/AST/Module.h b/include/swift/AST/Module.h index f920eb09158..3a2b98384c3 100644 --- a/include/swift/AST/Module.h +++ b/include/swift/AST/Module.h @@ -872,10 +872,6 @@ public: return getParentModule()->getName().str(); } - /// If this is a module imported from a parseable interface, return the path - /// to the interface file, otherwise an empty StringRef. - virtual StringRef getParseableInterface() const { return {}; } - /// Traverse the decls within this file. /// /// \returns true if traversal was aborted, false if it completed diff --git a/include/swift/Serialization/ModuleFile.h b/include/swift/Serialization/ModuleFile.h index 898062f3f04..72d15933461 100644 --- a/include/swift/Serialization/ModuleFile.h +++ b/include/swift/Serialization/ModuleFile.h @@ -78,6 +78,11 @@ class ModuleFile /// The target the module was built for. StringRef TargetTriple; + /// The name of the module interface this module was compiled from. + /// + /// Empty if this module didn't come from an interface file. + StringRef ModuleInterfacePath; + /// The Swift compatibility version in use when this module was built. version::Version CompatibilityVersion; @@ -762,6 +767,8 @@ public: void getDisplayDecls(SmallVectorImpl &results); StringRef getModuleFilename() const { + if (!ModuleInterfacePath.empty()) + return ModuleInterfacePath; // FIXME: This seems fragile, maybe store the filename separately ? return ModuleInputBuffer->getBufferIdentifier(); } diff --git a/include/swift/Serialization/SerializedModuleLoader.h b/include/swift/Serialization/SerializedModuleLoader.h index bc7880260fb..05af36846fd 100644 --- a/include/swift/Serialization/SerializedModuleLoader.h +++ b/include/swift/Serialization/SerializedModuleLoader.h @@ -251,10 +251,6 @@ class SerializedASTFile final : public LoadedFile { ModuleFile &File; - /// The parseable interface this module was generated from if any. - /// Used for debug info. - std::string ParseableInterface; - bool IsSIB; ~SerializedASTFile() = default; @@ -351,13 +347,6 @@ public: virtual const clang::Module *getUnderlyingClangModule() const override; - /// If this is a module imported from a parseable interface, return the path - /// to the interface file, otherwise an empty StringRef. - virtual StringRef getParseableInterface() const override { - return ParseableInterface; - } - void setParseableInterface(StringRef PI) { ParseableInterface = PI; } - virtual bool getAllGenericSignatures( SmallVectorImpl &genericSignatures) override; diff --git a/include/swift/Serialization/Validation.h b/include/swift/Serialization/Validation.h index 785a8e4d271..b07e689f48e 100644 --- a/include/swift/Serialization/Validation.h +++ b/include/swift/Serialization/Validation.h @@ -92,7 +92,6 @@ struct ValidationInfo { class ExtendedValidationInfo { SmallVector ExtraClangImporterOpts; StringRef SDKPath; - StringRef ParseableInterface; struct { unsigned ArePrivateImportsEnabled : 1; unsigned IsSIB : 1; @@ -114,8 +113,6 @@ public: void addExtraClangImporterOption(StringRef option) { ExtraClangImporterOpts.push_back(option); } - StringRef getParseableInterface() const { return ParseableInterface; } - void setParseableInterface(StringRef PI) { ParseableInterface = PI; } bool isSIB() const { return Bits.IsSIB; } void setIsSIB(bool val) { diff --git a/lib/AST/Module.cpp b/lib/AST/Module.cpp index a44d751cd2e..ddd5c7f87ea 100644 --- a/lib/AST/Module.cpp +++ b/lib/AST/Module.cpp @@ -1249,10 +1249,6 @@ StringRef ModuleDecl::getModuleFilename() const { // per-file names. Modules can consist of more than one file. StringRef Result; for (auto F : getFiles()) { - Result = F->getParseableInterface(); - if (!Result.empty()) - return Result; - if (auto SF = dyn_cast(F)) { if (!Result.empty()) return StringRef(); diff --git a/lib/Serialization/ModuleFile.cpp b/lib/Serialization/ModuleFile.cpp index 7ab30fa148e..c6ae21a6b92 100644 --- a/lib/Serialization/ModuleFile.cpp +++ b/lib/Serialization/ModuleFile.cpp @@ -1352,8 +1352,7 @@ ModuleFile::ModuleFile( break; } case input_block::PARSEABLE_INTERFACE_PATH: { - if (extInfo) - extInfo->setParseableInterface(blobData); + ModuleInterfacePath = blobData; break; } default: diff --git a/lib/Serialization/SerializedModuleLoader.cpp b/lib/Serialization/SerializedModuleLoader.cpp index 872b24df31b..93ac4b770a4 100644 --- a/lib/Serialization/SerializedModuleLoader.cpp +++ b/lib/Serialization/SerializedModuleLoader.cpp @@ -548,7 +548,6 @@ FileUnit *SerializedModuleLoaderBase::loadAST( // We've loaded the file. Now try to bring it into the AST. auto fileUnit = new (Ctx) SerializedASTFile(M, *loadedModuleFile, extendedInfo.isSIB()); - fileUnit->setParseableInterface(extendedInfo.getParseableInterface()); M.addFile(*fileUnit); if (extendedInfo.isTestable()) M.setTestingEnabled(); From 764e2b8ce6e337ca687dea197eb8c7269c408e66 Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Fri, 23 Aug 2019 17:25:44 -0700 Subject: [PATCH 3/4] Sink private-file import support down to SerializedASTFile Implementing it in LoadedFile is nice in theory, but causes a leak in practice because that type is ASTContext-allocated and usually never destroyed. https://bugs.swift.org/browse/SR-11366 --- include/swift/AST/Module.h | 18 ++---------------- include/swift/Serialization/ModuleFile.h | 1 + .../Serialization/SerializedModuleLoader.h | 3 +++ lib/Serialization/Deserialization.cpp | 10 ++++------ lib/Serialization/SerializedModuleLoader.cpp | 5 +++++ 5 files changed, 15 insertions(+), 22 deletions(-) diff --git a/include/swift/AST/Module.h b/include/swift/AST/Module.h index 3a2b98384c3..43d48a7c459 100644 --- a/include/swift/AST/Module.h +++ b/include/swift/AST/Module.h @@ -1387,28 +1387,14 @@ protected: : FileUnit(Kind, M) { assert(classof(this) && "invalid kind"); } - - /// A map from private/fileprivate decls to the file they were defined in. - llvm::DenseMap FilenameForPrivateDecls; - public: - /// Returns an arbitrary string representing the storage backing this file. /// /// This is usually a filesystem path. virtual StringRef getFilename() const; - void addFilenameForPrivateDecl(const ValueDecl *decl, Identifier id) { - assert(!FilenameForPrivateDecls.count(decl) || - FilenameForPrivateDecls[decl] == id); - FilenameForPrivateDecls[decl] = id; - } - - StringRef getFilenameForPrivateDecl(const ValueDecl *decl) { - auto it = FilenameForPrivateDecls.find(decl); - if (it == FilenameForPrivateDecls.end()) - return StringRef(); - return it->second.str(); + virtual StringRef getFilenameForPrivateDecl(const ValueDecl *decl) const { + return StringRef(); } /// Look up an operator declaration. diff --git a/include/swift/Serialization/ModuleFile.h b/include/swift/Serialization/ModuleFile.h index 72d15933461..e16520eb9dc 100644 --- a/include/swift/Serialization/ModuleFile.h +++ b/include/swift/Serialization/ModuleFile.h @@ -396,6 +396,7 @@ private: std::unique_ptr ObjCMethods; llvm::DenseMap PrivateDiscriminatorsByValue; + llvm::DenseMap FilenamesForPrivateValues; TinyPtrVector ImportDecls; diff --git a/include/swift/Serialization/SerializedModuleLoader.h b/include/swift/Serialization/SerializedModuleLoader.h index 05af36846fd..092e7da2dcd 100644 --- a/include/swift/Serialization/SerializedModuleLoader.h +++ b/include/swift/Serialization/SerializedModuleLoader.h @@ -274,6 +274,9 @@ public: DeclName name, NLKind lookupKind, SmallVectorImpl &results) const override; + virtual StringRef + getFilenameForPrivateDecl(const ValueDecl *decl) const override; + virtual TypeDecl *lookupLocalType(StringRef MangledName) const override; virtual OpaqueTypeDecl * diff --git a/lib/Serialization/Deserialization.cpp b/lib/Serialization/Deserialization.cpp index b276f779f8e..4c7dcbcb695 100644 --- a/lib/Serialization/Deserialization.cpp +++ b/lib/Serialization/Deserialization.cpp @@ -2254,7 +2254,7 @@ class swift::DeclDeserializer { Identifier privateDiscriminator; unsigned localDiscriminator = 0; - Identifier filenameForPrivate; + StringRef filenameForPrivate; void AddAttribute(DeclAttribute *Attr) { // Advance the linked list. @@ -2308,10 +2308,8 @@ public: if (localDiscriminator != 0) value->setLocalDiscriminator(localDiscriminator); - if (!filenameForPrivate.empty()) { - auto *loadedFile = cast(MF.getFile()); - loadedFile->addFilenameForPrivateDecl(value, filenameForPrivate); - } + if (!filenameForPrivate.empty()) + MF.FilenamesForPrivateValues[value] = filenameForPrivate; } decl->setValidationToChecked(); @@ -4202,7 +4200,7 @@ llvm::Error DeclDeserializer::deserializeDeclAttributes() { } else if (recordID == decls_block::FILENAME_FOR_PRIVATE) { IdentifierID filenameID; decls_block::FilenameForPrivateLayout::readRecord(scratch, filenameID); - filenameForPrivate = MF.getIdentifier(filenameID); + filenameForPrivate = MF.getIdentifierText(filenameID); } else { return llvm::Error::success(); } diff --git a/lib/Serialization/SerializedModuleLoader.cpp b/lib/Serialization/SerializedModuleLoader.cpp index 93ac4b770a4..fbc5d8f4c26 100644 --- a/lib/Serialization/SerializedModuleLoader.cpp +++ b/lib/Serialization/SerializedModuleLoader.cpp @@ -955,6 +955,11 @@ void SerializedASTFile::lookupValue(ModuleDecl::AccessPathTy accessPath, File.lookupValue(name, results); } +StringRef +SerializedASTFile::getFilenameForPrivateDecl(const ValueDecl *decl) const { + return File.FilenamesForPrivateValues.lookup(decl); +} + TypeDecl *SerializedASTFile::lookupLocalType(llvm::StringRef MangledName) const{ return File.lookupLocalType(MangledName); } From 5c785d42b3fddad3fa3531a31433e04b402b835a Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Fri, 23 Aug 2019 17:28:02 -0700 Subject: [PATCH 4/4] Enforce that FileUnit + LoadedFile have trivial destructors We already do this for other ASTContext-allocated types (see Decl.cpp). This will prevent the sort of mistakes in the previous two commits. Note that if any particular subclass of FileUnit wants to have its destructor run, it can opt into that manually using ASTContext::addDestructorCleanup. SourceFile and BuiltinUnit both do this. But we generally don't /want/ to do this if we can avoid it because it adds to compiler teardown time. --- include/swift/AST/Module.h | 17 +++++++---------- include/swift/ClangImporter/ClangModule.h | 2 -- .../Serialization/SerializedModuleLoader.h | 2 -- lib/AST/Module.cpp | 5 +++++ lib/ClangImporter/ClangImporter.cpp | 3 +++ lib/ClangImporter/DWARFImporter.cpp | 4 +++- lib/Serialization/ModuleFile.cpp | 3 +++ 7 files changed, 21 insertions(+), 15 deletions(-) diff --git a/include/swift/AST/Module.h b/include/swift/AST/Module.h index 43d48a7c459..dd51ba6e90f 100644 --- a/include/swift/AST/Module.h +++ b/include/swift/AST/Module.h @@ -644,7 +644,10 @@ static inline unsigned alignOfFileUnit(); /// FileUnit is an abstract base class; its subclasses represent different /// sorts of containers that can each provide a set of decls, e.g. a source /// file. A module can contain several file-units. +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wnon-virtual-dtor" class FileUnit : public DeclContext { +#pragma clang diagnostic pop virtual void anchor(); // FIXME: Stick this in a PointerIntPair. @@ -655,8 +658,6 @@ protected: : DeclContext(DeclContextKind::FileUnit, &M), Kind(kind) { } - virtual ~FileUnit() = default; - public: FileUnitKind getKind() const { return Kind; @@ -891,13 +892,7 @@ private: // Make placement new and vanilla new/delete illegal for FileUnits. void *operator new(size_t Bytes) throw() = delete; void *operator new(size_t Bytes, void *Mem) throw() = delete; - -protected: - // Unfortunately we can't remove this altogether because the virtual - // destructor requires it to be accessible. - void operator delete(void *Data) throw() { - llvm_unreachable("Don't use operator delete on a SourceFile"); - } + void operator delete(void *Data) throw() = delete; public: // Only allow allocation of FileUnits using the allocator in ASTContext @@ -1380,9 +1375,11 @@ public: }; /// Represents an externally-loaded file of some kind. +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wnon-virtual-dtor" class LoadedFile : public FileUnit { +#pragma clang diagnostic pop protected: - ~LoadedFile() = default; LoadedFile(FileUnitKind Kind, ModuleDecl &M) noexcept : FileUnit(Kind, M) { assert(classof(this) && "invalid kind"); diff --git a/include/swift/ClangImporter/ClangModule.h b/include/swift/ClangImporter/ClangModule.h index 388d66b0b41..60ee6579d50 100644 --- a/include/swift/ClangImporter/ClangModule.h +++ b/include/swift/ClangImporter/ClangModule.h @@ -39,8 +39,6 @@ class ClangModuleUnit final : public LoadedFile { /// The metadata of the underlying Clang module. clang::ExternalASTSource::ASTSourceDescriptor ASTSourceDescriptor; - ~ClangModuleUnit() = default; - public: /// True if the given Module contains an imported Clang module unit. static bool hasClangModule(ModuleDecl *M); diff --git a/include/swift/Serialization/SerializedModuleLoader.h b/include/swift/Serialization/SerializedModuleLoader.h index 092e7da2dcd..b419cda8db6 100644 --- a/include/swift/Serialization/SerializedModuleLoader.h +++ b/include/swift/Serialization/SerializedModuleLoader.h @@ -253,8 +253,6 @@ class SerializedASTFile final : public LoadedFile { bool IsSIB; - ~SerializedASTFile() = default; - SerializedASTFile(ModuleDecl &M, ModuleFile &file, bool isSIB = false) : LoadedFile(FileUnitKind::SerializedAST, M), File(file), IsSIB(isSIB) {} diff --git a/lib/AST/Module.cpp b/lib/AST/Module.cpp index ddd5c7f87ea..ee5c74a906a 100644 --- a/lib/AST/Module.cpp +++ b/lib/AST/Module.cpp @@ -55,6 +55,11 @@ using namespace swift; +static_assert(IsTriviallyDestructible::value, + "FileUnits are BumpPtrAllocated; the d'tor may not be called"); +static_assert(IsTriviallyDestructible::value, + "LoadedFiles are BumpPtrAllocated; the d'tor may not be called"); + //===----------------------------------------------------------------------===// // Builtin Module Name lookup //===----------------------------------------------------------------------===// diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp index a2b8f36a0c9..28ab7ba5596 100644 --- a/lib/ClangImporter/ClangImporter.cpp +++ b/lib/ClangImporter/ClangImporter.cpp @@ -3176,6 +3176,9 @@ void ClangImporter::verifyAllModules() { // ClangModule Implementation //===----------------------------------------------------------------------===// +static_assert(IsTriviallyDestructible::value, + "ClangModuleUnits are BumpPtrAllocated; the d'tor is not called"); + ClangModuleUnit::ClangModuleUnit(ModuleDecl &M, ClangImporter::Implementation &owner, const clang::Module *clangModule) diff --git a/lib/ClangImporter/DWARFImporter.cpp b/lib/ClangImporter/DWARFImporter.cpp index 582295980a0..0ac6d2e63ee 100644 --- a/lib/ClangImporter/DWARFImporter.cpp +++ b/lib/ClangImporter/DWARFImporter.cpp @@ -20,7 +20,6 @@ void DWARFImporterDelegate::anchor() {} /// Represents a Clang module that was "imported" from debug info. Since all the /// loading of types is done on demand, this class is effectively empty. class DWARFModuleUnit final : public LoadedFile { - ~DWARFModuleUnit() = default; ClangImporter::Implementation &Owner; public: @@ -96,6 +95,9 @@ public: } }; +static_assert(IsTriviallyDestructible::value, + "DWARFModuleUnits are BumpPtrAllocated; the d'tor is not called"); + ModuleDecl *ClangImporter::Implementation::loadModuleDWARF( SourceLoc importLoc, ArrayRef> path) { // There's no importing from debug info if no importer is installed. diff --git a/lib/Serialization/ModuleFile.cpp b/lib/Serialization/ModuleFile.cpp index c6ae21a6b92..1492f78b446 100644 --- a/lib/Serialization/ModuleFile.cpp +++ b/lib/Serialization/ModuleFile.cpp @@ -38,6 +38,9 @@ using namespace swift::serialization; using namespace llvm::support; using llvm::Expected; +static_assert(IsTriviallyDestructible::value, + "SerializedASTFiles are BumpPtrAllocated; d'tors are not called"); + static bool checkModuleSignature(llvm::BitstreamCursor &cursor, ArrayRef signature) { for (unsigned char byte : signature)