From 6192ad262239934df111ca5297dcc48bec597a22 Mon Sep 17 00:00:00 2001 From: Raphael Isemann Date: Fri, 20 Sep 2019 12:52:55 +0000 Subject: [PATCH] Move decl completion out of the ASTImporterDelegate and document it [NFC] Summary: The ASTImporterDelegate is currently responsible for both recording and also completing types. This patch moves the actual completion and recording code outside the ASTImporterDelegate to reduce the amount of responsibilities the ASTImporterDelegate has to fulfill. As I anyway had to touch the code when moving I also documented and refactored most of it (e.g. no more asserts that we call the deporting start/end function always as a pair). Note that I had to make the ASTImporterDelegate and it's related functions public now so that I can move out the functionality in another class (that doesn't need to be in the header). Reviewers: shafik, aprantl, martong, a.sidorin Reviewed By: martong Subscribers: rnkovacs, lldb-commits Tags: #lldb Differential Revision: https://reviews.llvm.org/D61478 llvm-svn: 372385 --- lldb/include/lldb/Symbol/ClangASTImporter.h | 54 ++++---- lldb/source/Symbol/ClangASTImporter.cpp | 202 ++++++++++++++-------------- 2 files changed, 127 insertions(+), 129 deletions(-) diff --git a/lldb/include/lldb/Symbol/ClangASTImporter.h b/lldb/include/lldb/Symbol/ClangASTImporter.h index eb36894..f963f9b 100644 --- a/lldb/include/lldb/Symbol/ClangASTImporter.h +++ b/lldb/include/lldb/Symbol/ClangASTImporter.h @@ -210,7 +210,7 @@ public: void ForgetDestination(clang::ASTContext *dst_ctx); void ForgetSource(clang::ASTContext *dst_ctx, clang::ASTContext *src_ctx); -private: +public: struct DeclOrigin { DeclOrigin() : ctx(nullptr), decl(nullptr) {} @@ -235,23 +235,29 @@ private: typedef std::map OriginMap; + /// Listener interface used by the ASTImporterDelegate to inform other code + /// about decls that have been imported the first time. + struct NewDeclListener { + virtual ~NewDeclListener() = default; + /// A decl has been imported for the first time. + virtual void NewDeclImported(clang::Decl *from, clang::Decl *to) = 0; + }; + /// ASTImporter that intercepts and records the import process of the /// underlying ASTImporter. /// /// This class updates the map from declarations to their original - /// declarations and can record and complete declarations that have been - /// imported in a certain interval. + /// declarations and can record declarations that have been imported in a + /// certain interval. /// /// When intercepting a declaration import, the ASTImporterDelegate uses the /// CxxModuleHandler to replace any missing or malformed declarations with /// their counterpart from a C++ module. - class ASTImporterDelegate : public clang::ASTImporter { - public: + struct ASTImporterDelegate : public clang::ASTImporter { ASTImporterDelegate(ClangASTImporter &master, clang::ASTContext *target_ctx, clang::ASTContext *source_ctx) : clang::ASTImporter(*target_ctx, master.m_file_manager, *source_ctx, master.m_file_manager, true /*minimal*/), - m_decls_to_deport(nullptr), m_decls_already_deported(nullptr), m_master(master), m_source_ctx(source_ctx) { setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal); } @@ -287,43 +293,32 @@ private: } }; - protected: - llvm::Expected ImportImpl(clang::Decl *From) override; - - public: - // A call to "InitDeportWorkQueues" puts the delegate into deport mode. - // In deport mode, every copied Decl that could require completion is - // recorded and placed into the decls_to_deport set. - // - // A call to "ExecuteDeportWorkQueues" completes all the Decls that - // are in decls_to_deport, adding any Decls it sees along the way that it - // hasn't already deported. It proceeds until decls_to_deport is empty. - // - // These calls must be paired. Leaving a delegate in deport mode or trying - // to start deport delegate with a new pair of queues will result in an - // assertion failure. - - void - InitDeportWorkQueues(std::set *decls_to_deport, - std::set *decls_already_deported); - void ExecuteDeportWorkQueues(); - void ImportDefinitionTo(clang::Decl *to, clang::Decl *from); void Imported(clang::Decl *from, clang::Decl *to) override; clang::Decl *GetOriginalDecl(clang::Decl *To) override; + void SetImportListener(NewDeclListener *listener) { + assert(m_new_decl_listener == nullptr && "Already attached a listener?"); + m_new_decl_listener = listener; + } + void RemoveImportListener() { m_new_decl_listener = nullptr; } + + protected: + llvm::Expected ImportImpl(clang::Decl *From) override; + + private: /// Decls we should ignore when mapping decls back to their original /// ASTContext. Used by the CxxModuleHandler to mark declarations that /// were created from the 'std' C++ module to prevent that the Importer /// tries to sync them with the broken equivalent in the debug info AST. std::set m_decls_to_ignore; - std::set *m_decls_to_deport; - std::set *m_decls_already_deported; ClangASTImporter &m_master; clang::ASTContext *m_source_ctx; CxxModuleHandler *m_std_handler = nullptr; + /// The currently attached listener. + NewDeclListener *m_new_decl_listener = nullptr; }; typedef std::shared_ptr ImporterDelegateSP; @@ -389,6 +384,7 @@ private: } } +public: DeclOrigin GetDeclOrigin(const clang::Decl *decl); clang::FileManager m_file_manager; diff --git a/lldb/source/Symbol/ClangASTImporter.cpp b/lldb/source/Symbol/ClangASTImporter.cpp index 5319f79..92d51f7 100644 --- a/lldb/source/Symbol/ClangASTImporter.cpp +++ b/lldb/source/Symbol/ClangASTImporter.cpp @@ -250,6 +250,94 @@ public: } }; +namespace { +/// Completes all imported TagDecls at the end of the scope. +/// +/// While in a CompleteTagDeclsScope, every decl that could be completed will +/// be completed at the end of the scope (including all Decls that are +/// imported while completing the original Decls). +class CompleteTagDeclsScope : public ClangASTImporter::NewDeclListener { + ClangASTImporter::ImporterDelegateSP m_delegate; + // FIXME: Investigate how many decls we usually have in these sets and + // see if we can use SmallPtrSet instead here. + std::set m_decls_to_complete; + std::set m_decls_already_completed; + clang::ASTContext *m_dst_ctx; + clang::ASTContext *m_src_ctx; + ClangASTImporter &importer; + +public: + /// Constructs a CompleteTagDeclsScope. + /// \param importer The ClangASTImporter that we should observe. + /// \param dst_ctx The ASTContext to which Decls are imported. + /// \param src_ctx The ASTContext from which Decls are imported. + explicit CompleteTagDeclsScope(ClangASTImporter &importer, + clang::ASTContext *dst_ctx, + clang::ASTContext *src_ctx) + : m_delegate(importer.GetDelegate(dst_ctx, src_ctx)), m_dst_ctx(dst_ctx), + m_src_ctx(src_ctx), importer(importer) { + m_delegate->SetImportListener(this); + } + + virtual ~CompleteTagDeclsScope() { + ClangASTImporter::ASTContextMetadataSP to_context_md = + importer.GetContextMetadata(m_dst_ctx); + + // Complete all decls we collected until now. + while (!m_decls_to_complete.empty()) { + NamedDecl *decl = *m_decls_to_complete.begin(); + + m_decls_already_completed.insert(decl); + m_decls_to_complete.erase(decl); + + // We should only complete decls coming from the source context. + assert(to_context_md->m_origins[decl].ctx == m_src_ctx); + + Decl *original_decl = to_context_md->m_origins[decl].decl; + + // Complete the decl now. + ClangASTContext::GetCompleteDecl(m_src_ctx, original_decl); + if (auto *tag_decl = dyn_cast(decl)) { + if (auto *original_tag_decl = dyn_cast(original_decl)) { + if (original_tag_decl->isCompleteDefinition()) { + m_delegate->ImportDefinitionTo(tag_decl, original_tag_decl); + tag_decl->setCompleteDefinition(true); + } + } + + tag_decl->setHasExternalLexicalStorage(false); + tag_decl->setHasExternalVisibleStorage(false); + } else if (auto *container_decl = dyn_cast(decl)) { + container_decl->setHasExternalLexicalStorage(false); + container_decl->setHasExternalVisibleStorage(false); + } + + to_context_md->m_origins.erase(decl); + } + + // Stop listening to imported decls. We do this after clearing the + // Decls we needed to import to catch all Decls they might have pulled in. + m_delegate->RemoveImportListener(); + } + + void NewDeclImported(clang::Decl *from, clang::Decl *to) override { + // Filter out decls that we can't complete later. + if (!isa(to) && !isa(to)) + return; + RecordDecl *from_record_decl = dyn_cast(from); + // We don't need to complete injected class name decls. + if (from_record_decl && from_record_decl->isInjectedClassName()) + return; + + NamedDecl *to_named_decl = dyn_cast(to); + // Check if we already completed this type. + if (m_decls_already_completed.count(to_named_decl) != 0) + return; + m_decls_to_complete.insert(to_named_decl); + } +}; +} // namespace + lldb::opaque_compiler_type_t ClangASTImporter::DeportType(clang::ASTContext *dst_ctx, clang::ASTContext *src_ctx, @@ -263,27 +351,16 @@ ClangASTImporter::DeportType(clang::ASTContext *dst_ctx, (unsigned long long)type, static_cast(src_ctx), static_cast(dst_ctx)); - ImporterDelegateSP delegate_sp(GetDelegate(dst_ctx, src_ctx)); - - if (!delegate_sp) - return nullptr; - - std::set decls_to_deport; - std::set decls_already_deported; - DeclContextOverride decl_context_override; - if (const clang::TagType *tag_type = - clang::QualType::getFromOpaquePtr(type)->getAs()) { - decl_context_override.OverrideAllDeclsFromContainingFunction( - tag_type->getDecl()); - } - - delegate_sp->InitDeportWorkQueues(&decls_to_deport, &decls_already_deported); - - lldb::opaque_compiler_type_t result = CopyType(dst_ctx, src_ctx, type); + if (auto *t = QualType::getFromOpaquePtr(type)->getAs()) + decl_context_override.OverrideAllDeclsFromContainingFunction(t->getDecl()); - delegate_sp->ExecuteDeportWorkQueues(); + lldb::opaque_compiler_type_t result; + { + CompleteTagDeclsScope complete_scope(*this, dst_ctx, src_ctx); + result = CopyType(dst_ctx, src_ctx, type); + } if (!result) return nullptr; @@ -302,23 +379,15 @@ clang::Decl *ClangASTImporter::DeportDecl(clang::ASTContext *dst_ctx, decl->getDeclKindName(), static_cast(decl), static_cast(src_ctx), static_cast(dst_ctx)); - ImporterDelegateSP delegate_sp(GetDelegate(dst_ctx, src_ctx)); - - if (!delegate_sp) - return nullptr; - - std::set decls_to_deport; - std::set decls_already_deported; - DeclContextOverride decl_context_override; decl_context_override.OverrideAllDeclsFromContainingFunction(decl); - delegate_sp->InitDeportWorkQueues(&decls_to_deport, &decls_already_deported); - - clang::Decl *result = CopyDecl(dst_ctx, src_ctx, decl); - - delegate_sp->ExecuteDeportWorkQueues(); + clang::Decl *result; + { + CompleteTagDeclsScope complete_scope(*this, dst_ctx, src_ctx); + result = CopyDecl(dst_ctx, src_ctx, decl); + } if (!result) return nullptr; @@ -870,63 +939,6 @@ ClangASTImporter::ASTImporterDelegate::ImportImpl(Decl *From) { return ASTImporter::ImportImpl(From); } -void ClangASTImporter::ASTImporterDelegate::InitDeportWorkQueues( - std::set *decls_to_deport, - std::set *decls_already_deported) { - assert(!m_decls_to_deport); - assert(!m_decls_already_deported); - - m_decls_to_deport = decls_to_deport; - m_decls_already_deported = decls_already_deported; -} - -void ClangASTImporter::ASTImporterDelegate::ExecuteDeportWorkQueues() { - assert(m_decls_to_deport); - assert(m_decls_already_deported); - - ASTContextMetadataSP to_context_md = - m_master.GetContextMetadata(&getToContext()); - - while (!m_decls_to_deport->empty()) { - NamedDecl *decl = *m_decls_to_deport->begin(); - - m_decls_already_deported->insert(decl); - m_decls_to_deport->erase(decl); - - DeclOrigin &origin = to_context_md->m_origins[decl]; - UNUSED_IF_ASSERT_DISABLED(origin); - - assert(origin.ctx == - m_source_ctx); // otherwise we should never have added this - // because it doesn't need to be deported - - Decl *original_decl = to_context_md->m_origins[decl].decl; - - ClangASTContext::GetCompleteDecl(m_source_ctx, original_decl); - - if (TagDecl *tag_decl = dyn_cast(decl)) { - if (TagDecl *original_tag_decl = dyn_cast(original_decl)) { - if (original_tag_decl->isCompleteDefinition()) { - ImportDefinitionTo(tag_decl, original_tag_decl); - tag_decl->setCompleteDefinition(true); - } - } - - tag_decl->setHasExternalLexicalStorage(false); - tag_decl->setHasExternalVisibleStorage(false); - } else if (ObjCContainerDecl *container_decl = - dyn_cast(decl)) { - container_decl->setHasExternalLexicalStorage(false); - container_decl->setHasExternalVisibleStorage(false); - } - - to_context_md->m_origins.erase(decl); - } - - m_decls_to_deport = nullptr; - m_decls_already_deported = nullptr; -} - void ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo( clang::Decl *to, clang::Decl *from) { ASTImporter::Imported(from, to); @@ -1092,18 +1104,8 @@ void ClangASTImporter::ASTImporterDelegate::Imported(clang::Decl *from, static_cast(&from->getASTContext()), static_cast(&to->getASTContext())); } else { - if (m_decls_to_deport && m_decls_already_deported) { - if (isa(to) || isa(to)) { - RecordDecl *from_record_decl = dyn_cast(from); - if (from_record_decl == nullptr || - !from_record_decl->isInjectedClassName()) { - NamedDecl *to_named_decl = dyn_cast(to); - - if (!m_decls_already_deported->count(to_named_decl)) - m_decls_to_deport->insert(to_named_decl); - } - } - } + if (m_new_decl_listener) + m_new_decl_listener->NewDeclImported(from, to); if (to_context_md->m_origins.find(to) == to_context_md->m_origins.end() || user_id != LLDB_INVALID_UID) { -- 2.7.4