From 360d901bf0478293d6e57f58bdb63b386f97f531 Mon Sep 17 00:00:00 2001 From: Jordan Rupprecht Date: Wed, 10 Nov 2021 10:31:30 -0800 Subject: [PATCH] Revert "[lldb] Disable minimal import mode for RecordDecls that back FieldDecls" This reverts commit 3bf96b0329be554c67282b0d7d8da6a864b9e38f. It causes crashes as reported in PR52257 and a few other places. A reproducer is bundled with this commit to verify any fix forward. The original test is left in place, but marked XFAIL as it now produces the wrong result. Reviewed By: teemperor Differential Revision: https://reviews.llvm.org/D113449 --- .../Clang/ClangASTImporter.cpp | 31 ------------------- .../ExpressionParser/Clang/ClangASTSource.cpp | 11 ++++++- .../API/commands/expression/pr52257/Makefile | 3 ++ .../expression/pr52257/TestExprCrash.py | 18 +++++++++++ .../API/commands/expression/pr52257/main.cpp | 12 +++++++ .../TestCppReferenceToOuterClass.py | 1 + 6 files changed, 44 insertions(+), 32 deletions(-) create mode 100644 lldb/test/API/commands/expression/pr52257/Makefile create mode 100644 lldb/test/API/commands/expression/pr52257/TestExprCrash.py create mode 100644 lldb/test/API/commands/expression/pr52257/main.cpp diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp index 94647b0ef978..9ff6fbc28bf9 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp @@ -888,37 +888,6 @@ ClangASTImporter::ASTImporterDelegate::ImportImpl(Decl *From) { LLDB_LOG(log, "[ClangASTImporter] Complete definition not found"); } - // Disable the minimal import for fields that have record types. There is - // no point in minimally importing the record behind their type as Clang - // will anyway request their definition when the FieldDecl is added to the - // RecordDecl (as Clang will query the FieldDecl's type for things such - // as a deleted constexpr destructor). - // By importing the type ahead of time we avoid some corner cases where - // the FieldDecl's record is importing in the middle of Clang's - // `DeclContext::addDecl` logic. - if (clang::FieldDecl *fd = dyn_cast(From)) { - // This is only necessary because we do the 'minimal import'. Remove this - // once LLDB stopped using that mode. - assert(isMinimalImport() && "Only necessary for minimal import"); - QualType field_type = fd->getType(); - if (field_type->isRecordType()) { - // First get the underlying record and minimally import it. - clang::TagDecl *record_decl = field_type->getAsTagDecl(); - llvm::Expected imported = Import(record_decl); - if (!imported) - return imported.takeError(); - // Check how/if the import got redirected to a different AST. Now - // import the definition of what was actually imported. If there is no - // origin then that means the record was imported by just picking a - // compatible type in the target AST (in which case there is no more - // importing to do). - if (clang::Decl *origin = m_master.GetDeclOrigin(*imported).decl) { - if (llvm::Error def_err = ImportDefinition(record_decl)) - return std::move(def_err); - } - } - } - return ASTImporter::ImportImpl(From); } diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp index ae93252e5521..410d8a95cb12 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp @@ -479,7 +479,10 @@ void ClangASTSource::FindExternalLexicalDecls( decl->getDeclKindName(), ast_dump); } - CopyDecl(decl); + Decl *copied_decl = CopyDecl(decl); + + if (!copied_decl) + continue; // FIXME: We should add the copied decl to the 'decls' list. This would // add the copied Decl into the DeclContext and make sure that we @@ -489,6 +492,12 @@ void ClangASTSource::FindExternalLexicalDecls( // lookup issues later on. // We can't just add them for now as the ASTImporter already added the // decl into the DeclContext and this would add it twice. + + if (FieldDecl *copied_field = dyn_cast(copied_decl)) { + QualType copied_field_type = copied_field->getType(); + + m_ast_importer_sp->RequireCompleteType(copied_field_type); + } } else { SkippedDecls = true; } diff --git a/lldb/test/API/commands/expression/pr52257/Makefile b/lldb/test/API/commands/expression/pr52257/Makefile new file mode 100644 index 000000000000..99998b20bcb0 --- /dev/null +++ b/lldb/test/API/commands/expression/pr52257/Makefile @@ -0,0 +1,3 @@ +CXX_SOURCES := main.cpp + +include Makefile.rules diff --git a/lldb/test/API/commands/expression/pr52257/TestExprCrash.py b/lldb/test/API/commands/expression/pr52257/TestExprCrash.py new file mode 100644 index 000000000000..68e5323ac35e --- /dev/null +++ b/lldb/test/API/commands/expression/pr52257/TestExprCrash.py @@ -0,0 +1,18 @@ +""" +Verify that LLDB doesn't crash during expression evaluation. +""" + +import lldb +from lldbsuite.test.decorators import * +from lldbsuite.test.lldbtest import * +from lldbsuite.test import lldbutil + + +class ExprCrashTestCase(TestBase): + + mydir = TestBase.compute_mydir(__file__) + + def test_pr52257(self): + self.build() + self.createTestTarget() + self.expect_expr("b", result_type="B", result_children=[ValueCheck(name="tag_set_")]) diff --git a/lldb/test/API/commands/expression/pr52257/main.cpp b/lldb/test/API/commands/expression/pr52257/main.cpp new file mode 100644 index 000000000000..7dcdc183053b --- /dev/null +++ b/lldb/test/API/commands/expression/pr52257/main.cpp @@ -0,0 +1,12 @@ +template struct pair {}; +struct A { + using iterator = pair; + pair a_[]; +}; +struct B { + using iterator = A::iterator; + iterator begin(); + A *tag_set_; +}; +B b; +int main() {}; diff --git a/lldb/test/API/lang/cpp/reference-to-outer-type/TestCppReferenceToOuterClass.py b/lldb/test/API/lang/cpp/reference-to-outer-type/TestCppReferenceToOuterClass.py index 0db28c4641e8..f9ba26ab32ba 100644 --- a/lldb/test/API/lang/cpp/reference-to-outer-type/TestCppReferenceToOuterClass.py +++ b/lldb/test/API/lang/cpp/reference-to-outer-type/TestCppReferenceToOuterClass.py @@ -7,6 +7,7 @@ class TestCase(TestBase): mydir = TestBase.compute_mydir(__file__) + @expectedFailure("The fix for this was reverted due to llvm.org/PR52257") def test(self): self.build() self.dbg.CreateTarget(self.getBuildArtifact("a.out"))