[Lldb-commits] [lldb] 360d901 - Revert "[lldb] Disable minimal import mode for RecordDecls that back FieldDecls"
Jordan Rupprecht via lldb-commits
lldb-commits at lists.llvm.org
Wed Nov 10 10:38:53 PST 2021
Author: Jordan Rupprecht
Date: 2021-11-10T10:38:01-08:00
New Revision: 360d901bf0478293d6e57f58bdb63b386f97f531
URL: https://github.com/llvm/llvm-project/commit/360d901bf0478293d6e57f58bdb63b386f97f531
DIFF: https://github.com/llvm/llvm-project/commit/360d901bf0478293d6e57f58bdb63b386f97f531.diff
LOG: 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
Added:
lldb/test/API/commands/expression/pr52257/Makefile
lldb/test/API/commands/expression/pr52257/TestExprCrash.py
lldb/test/API/commands/expression/pr52257/main.cpp
Modified:
lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
lldb/test/API/lang/cpp/reference-to-outer-type/TestCppReferenceToOuterClass.py
Removed:
################################################################################
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
index 94647b0ef9785..9ff6fbc28bf9c 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<FieldDecl>(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<Decl *> imported = Import(record_decl);
- if (!imported)
- return imported.takeError();
- // Check how/if the import got redirected to a
diff erent 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 ae93252e55212..410d8a95cb120 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<FieldDecl>(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 0000000000000..99998b20bcb05
--- /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 0000000000000..68e5323ac35eb
--- /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 0000000000000..7dcdc183053bb
--- /dev/null
+++ b/lldb/test/API/commands/expression/pr52257/main.cpp
@@ -0,0 +1,12 @@
+template <typename> struct pair {};
+struct A {
+ using iterator = pair<char *>;
+ pair<char *> 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 0db28c4641e87..f9ba26ab32bab 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"))
More information about the lldb-commits
mailing list