From 2eb963abff8e65f79e5c4ac0787ebd65d501e3dd Mon Sep 17 00:00:00 2001 From: Raphael Isemann Date: Wed, 2 Oct 2019 12:26:08 +0000 Subject: [PATCH] [lldb][NFC] Create the ASTContext in ClangASTContext exactly once. Reason for this patch is the Ssame reason as for the previous patches: Having a ClangASTContext and being able to switch the associated ASTContext isn't a use case we have (or should have), so let's simplify all this code. This way it becomes clearer in what order we initialize data structures. The DWARFASTParserClangTests changes are necessary as the test is using a ClangASTContext but relied on the fact that no called function ever calls getASTContext() on our ClangASTContext (as that would create the ASTContext). As we now always create the ASTContext the fact that we had an uninitialized FileSystem made the test crash. llvm-svn: 373457 --- lldb/include/lldb/Symbol/ClangASTContext.h | 10 +- lldb/source/Symbol/ClangASTContext.cpp | 114 +++++++++++---------- .../SymbolFile/DWARF/DWARFASTParserClangTests.cpp | 17 ++- 3 files changed, 78 insertions(+), 63 deletions(-) diff --git a/lldb/include/lldb/Symbol/ClangASTContext.h b/lldb/include/lldb/Symbol/ClangASTContext.h index d791f18..f91122b 100644 --- a/lldb/include/lldb/Symbol/ClangASTContext.h +++ b/lldb/include/lldb/Symbol/ClangASTContext.h @@ -53,6 +53,7 @@ public: // Constructors and Destructors explicit ClangASTContext(llvm::StringRef triple = ""); + explicit ClangASTContext(ArchSpec arch); /// Constructs a ClangASTContext that uses an existing ASTContext internally. /// Useful when having an existing ASTContext created by Clang. @@ -114,10 +115,6 @@ public: const char *GetTargetTriple(); - void SetTargetTriple(llvm::StringRef target_triple); - - void SetArchitecture(const ArchSpec &arch); - void SetExternalSource( llvm::IntrusiveRefCntPtr &ast_source_up); @@ -1008,11 +1005,14 @@ private: // For ClangASTContext only ClangASTContext(const ClangASTContext &); const ClangASTContext &operator=(const ClangASTContext &); + /// Creates the internal ASTContext. + void CreateASTContext(); + void SetTargetTriple(llvm::StringRef target_triple); }; class ClangASTContextForExpressions : public ClangASTContext { public: - ClangASTContextForExpressions(Target &target); + ClangASTContextForExpressions(Target &target, ArchSpec arch); ~ClangASTContextForExpressions() override = default; diff --git a/lldb/source/Symbol/ClangASTContext.cpp b/lldb/source/Symbol/ClangASTContext.cpp index c928794..9795246 100644 --- a/lldb/source/Symbol/ClangASTContext.cpp +++ b/lldb/source/Symbol/ClangASTContext.cpp @@ -526,6 +526,17 @@ ClangASTContext::ClangASTContext(llvm::StringRef target_triple) : TypeSystem(TypeSystem::eKindClang) { if (!target_triple.empty()) SetTargetTriple(target_triple); + // The caller didn't pass an ASTContext so create a new one for this + // ClangASTContext. + CreateASTContext(); +} + +ClangASTContext::ClangASTContext(ArchSpec arch) + : TypeSystem(TypeSystem::eKindClang) { + SetTargetTriple(arch.GetTriple().str()); + // The caller didn't pass an ASTContext so create a new one for this + // ClangASTContext. + CreateASTContext(); } ClangASTContext::ClangASTContext(ASTContext &existing_ctxt) @@ -575,26 +586,21 @@ lldb::TypeSystemSP ClangASTContext::CreateInstance(lldb::LanguageType language, } if (module) { - std::shared_ptr ast_sp(new ClangASTContext); - if (ast_sp) { - ast_sp->SetArchitecture(fixed_arch); - } + std::shared_ptr ast_sp( + new ClangASTContext(fixed_arch)); return ast_sp; } else if (target && target->IsValid()) { std::shared_ptr ast_sp( - new ClangASTContextForExpressions(*target)); - if (ast_sp) { - ast_sp->SetArchitecture(fixed_arch); - ast_sp->m_scratch_ast_source_up.reset( - new ClangASTSource(target->shared_from_this())); - lldbassert(ast_sp->getFileManager()); - ast_sp->m_scratch_ast_source_up->InstallASTContext( - *ast_sp->getASTContext(), *ast_sp->getFileManager(), true); - llvm::IntrusiveRefCntPtr proxy_ast_source( - ast_sp->m_scratch_ast_source_up->CreateProxy()); - ast_sp->SetExternalSource(proxy_ast_source); - return ast_sp; - } + new ClangASTContextForExpressions(*target, fixed_arch)); + ast_sp->m_scratch_ast_source_up.reset( + new ClangASTSource(target->shared_from_this())); + lldbassert(ast_sp->getFileManager()); + ast_sp->m_scratch_ast_source_up->InstallASTContext( + *ast_sp->getASTContext(), *ast_sp->getFileManager(), true); + llvm::IntrusiveRefCntPtr proxy_ast_source( + ast_sp->m_scratch_ast_source_up->CreateProxy()); + ast_sp->SetExternalSource(proxy_ast_source); + return ast_sp; } } } @@ -638,11 +644,10 @@ void ClangASTContext::Terminate() { } void ClangASTContext::Finalize() { - if (m_ast_up) { - GetASTMap().Erase(m_ast_up.get()); - if (!m_ast_owned) - m_ast_up.release(); - } + assert(m_ast_up); + GetASTMap().Erase(m_ast_up.get()); + if (!m_ast_owned) + m_ast_up.release(); m_builtins_up.reset(); m_selector_table_up.reset(); @@ -652,12 +657,10 @@ void ClangASTContext::Finalize() { m_diagnostics_engine_up.reset(); m_source_manager_up.reset(); m_language_options_up.reset(); - m_ast_up.reset(); m_scratch_ast_source_up.reset(); } void ClangASTContext::Clear() { - m_ast_up.reset(); m_language_options_up.reset(); m_source_manager_up.reset(); m_diagnostics_engine_up.reset(); @@ -684,10 +687,6 @@ void ClangASTContext::SetTargetTriple(llvm::StringRef target_triple) { m_target_triple = target_triple.str(); } -void ClangASTContext::SetArchitecture(const ArchSpec &arch) { - SetTargetTriple(arch.GetTriple().str()); -} - void ClangASTContext::SetExternalSource( llvm::IntrusiveRefCntPtr &ast_source_up) { ASTContext *ast = getASTContext(); @@ -698,36 +697,39 @@ void ClangASTContext::SetExternalSource( } ASTContext *ClangASTContext::getASTContext() { - if (m_ast_up == nullptr) { - m_ast_owned = true; - m_ast_up.reset(new ASTContext(*getLanguageOptions(), *getSourceManager(), - *getIdentifierTable(), *getSelectorTable(), - *getBuiltinContext())); + assert(m_ast_up); + return m_ast_up.get(); +} - m_ast_up->getDiagnostics().setClient(getDiagnosticConsumer(), false); +void ClangASTContext::CreateASTContext() { + assert(!m_ast_up); + m_ast_owned = true; + m_ast_up.reset(new ASTContext(*getLanguageOptions(), *getSourceManager(), + *getIdentifierTable(), *getSelectorTable(), + *getBuiltinContext())); - // This can be NULL if we don't know anything about the architecture or if - // the target for an architecture isn't enabled in the llvm/clang that we - // built - TargetInfo *target_info = getTargetInfo(); - if (target_info) - m_ast_up->InitBuiltinTypes(*target_info); + m_ast_up->getDiagnostics().setClient(getDiagnosticConsumer(), false); - if ((m_callback_tag_decl || m_callback_objc_decl) && m_callback_baton) { - m_ast_up->getTranslationUnitDecl()->setHasExternalLexicalStorage(); - // m_ast_up->getTranslationUnitDecl()->setHasExternalVisibleStorage(); - } + // This can be NULL if we don't know anything about the architecture or if + // the target for an architecture isn't enabled in the llvm/clang that we + // built + TargetInfo *target_info = getTargetInfo(); + if (target_info) + m_ast_up->InitBuiltinTypes(*target_info); - GetASTMap().Insert(m_ast_up.get(), this); - - llvm::IntrusiveRefCntPtr ast_source_up( - new ClangExternalASTSourceCallbacks( - ClangASTContext::CompleteTagDecl, - ClangASTContext::CompleteObjCInterfaceDecl, nullptr, - ClangASTContext::LayoutRecordType, this)); - SetExternalSource(ast_source_up); + if ((m_callback_tag_decl || m_callback_objc_decl) && m_callback_baton) { + m_ast_up->getTranslationUnitDecl()->setHasExternalLexicalStorage(); + // m_ast_up->getTranslationUnitDecl()->setHasExternalVisibleStorage(); } - return m_ast_up.get(); + + GetASTMap().Insert(m_ast_up.get(), this); + + llvm::IntrusiveRefCntPtr ast_source_up( + new ClangExternalASTSourceCallbacks( + ClangASTContext::CompleteTagDecl, + ClangASTContext::CompleteObjCInterfaceDecl, nullptr, + ClangASTContext::LayoutRecordType, this)); + SetExternalSource(ast_source_up); } ClangASTContext *ClangASTContext::GetASTContext(clang::ASTContext *ast) { @@ -10242,9 +10244,9 @@ ClangASTContext::DeclContextGetClangASTContext(const CompilerDeclContext &dc) { return nullptr; } -ClangASTContextForExpressions::ClangASTContextForExpressions(Target &target) - : ClangASTContext(target.GetArchitecture().GetTriple().getTriple().c_str()), - m_target_wp(target.shared_from_this()), +ClangASTContextForExpressions::ClangASTContextForExpressions(Target &target, + ArchSpec arch) + : ClangASTContext(arch), m_target_wp(target.shared_from_this()), m_persistent_variables(new ClangPersistentVariables) {} UserExpression *ClangASTContextForExpressions::GetUserExpression( diff --git a/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp b/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp index 6c5d3b9..c68e351 100644 --- a/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp +++ b/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp @@ -15,6 +15,19 @@ using namespace lldb; using namespace lldb_private; +class DWARFASTParserClangTests : public testing::Test { +public: + void SetUp() override { + FileSystem::Initialize(); + ClangASTContext::Initialize(); + } + + void TearDown() override { + ClangASTContext::Terminate(); + FileSystem::Terminate(); + } +}; + namespace { class DWARFASTParserClangStub : public DWARFASTParserClang { public: @@ -32,8 +45,8 @@ public: // If your implementation needs to dereference the dummy pointers we are // defining here, causing this test to fail, feel free to delete it. -TEST(DWARFASTParserClangTests, - EnsureAllDIEsInDeclContextHaveBeenParsedParsesOnlyMatchingEntries) { +TEST_F(DWARFASTParserClangTests, + EnsureAllDIEsInDeclContextHaveBeenParsedParsesOnlyMatchingEntries) { ClangASTContext ast_ctx; DWARFASTParserClangStub ast_parser(ast_ctx); -- 2.7.4