This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3bf96b0329be: [lldb] Disable minimal import mode for
RecordDecls that back FieldDecls (authored by teemperor).
Herald added a subscriber: lldb-commits.
Changed prior to commit:
https://reviews.llvm.org/D102993?vs=347274&id=347618#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102993/new/
https://reviews.llvm.org/D102993
Files:
lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
lldb/test/API/lang/cpp/reference-to-outer-type/Makefile
lldb/test/API/lang/cpp/reference-to-outer-type/TestCppReferenceToOuterClass.py
lldb/test/API/lang/cpp/reference-to-outer-type/main.cpp
Index: lldb/test/API/lang/cpp/reference-to-outer-type/main.cpp
===================================================================
--- /dev/null
+++ lldb/test/API/lang/cpp/reference-to-outer-type/main.cpp
@@ -0,0 +1,23 @@
+struct Outer {
+ typedef int HookToOuter;
+ // When importing this type, we have to import all of it before adding it
+ // via the FieldDecl to 'Outer'. If we don't do this, then Clang will
+ // while adding 'NestedClassMember' ask for the full type of 'NestedClass'
+ // which then will start pulling in the 'RefToOuter' member. That member
+ // will import the typedef above and add it to 'Outer'. And adding a
+ // Decl to a DeclContext that is currently already in the process of adding
+ // another Decl will cause an inconsistent lookup.
+ struct NestedClass {
+ HookToOuter RefToOuter;
+ int SomeMember;
+ } NestedClassMember;
+};
+
+// We query the members of base classes of a type by doing a lookup via Clang.
+// As this tests is trying to find a borked lookup, we need a base class here
+// to make our 'GetChildMemberWithName' use the Clang lookup.
+struct In : Outer {};
+
+In test_var;
+
+int main() { return test_var.NestedClassMember.SomeMember; }
Index: lldb/test/API/lang/cpp/reference-to-outer-type/TestCppReferenceToOuterClass.py
===================================================================
--- /dev/null
+++ lldb/test/API/lang/cpp/reference-to-outer-type/TestCppReferenceToOuterClass.py
@@ -0,0 +1,16 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+ mydir = TestBase.compute_mydir(__file__)
+
+ def test(self):
+ self.build()
+ self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+ test_var = self.expect_expr("test_var", result_type="In")
+ nested_member = test_var.GetChildMemberWithName('NestedClassMember')
+ self.assertEqual("Outer::NestedClass",
+ nested_member.GetType().GetName())
Index: lldb/test/API/lang/cpp/reference-to-outer-type/Makefile
===================================================================
--- /dev/null
+++ lldb/test/API/lang/cpp/reference-to-outer-type/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===================================================================
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -479,10 +479,7 @@
decl->getDeclKindName(), ast_dump);
}
- Decl *copied_decl = CopyDecl(decl);
-
- if (!copied_decl)
- continue;
+ CopyDecl(decl);
// 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
@@ -492,12 +489,6 @@
// 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;
}
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
===================================================================
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
@@ -888,6 +888,37 @@
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 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);
}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
