shafik created this revision.
shafik added reviewers: martong, balazske, teemperor, a_sidorin.
Herald added a subscriber: rnkovacs.
Herald added a reviewer: a.sidorin.
shafik added a comment.
I am open to different approaches to this problem, this is opening attempt at
fixing it but I may be missing other interactions.
AFAICT starting the definition of `ToRecord` the way `CompleteDecl(…)` does
when we know `FromRecord` is not defined is just broken for the `RecordDecl`
case if we have bases.
It took me a lot of time to come up with this reproducer from a real life issue
debugging `llc`.
In `ImportContext(…)` we may call into `CompleteDecl(…)` which if `FromRecrord`
is not defined will start the definition of a `ToRecord` but from what I can
tell at least one of the paths though here don't ensure we complete the
definition.
For a `RecordDecl` this can be problematic since this means we won’t import
base classes and we won’t have any of the methods or types we inherit from
these bases.
One such path is through `VisitTypedefNameDecl(…)` which is exercised by the
reproducer.
For LLDB expression parsing this results in expressions failing but sometimes
succeeding in subsequent attempts e.g.:
(lldb) p BB->dump()
error: no member named 'dump' in 'B'
(lldb) p BB->dump()
(lldb)
This happens because the first time through `FromRecord` is not defined but in
subsequent attempts through it may be.
https://reviews.llvm.org/D78000
Files:
clang/lib/AST/ASTImporter.cpp
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
Index:
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===================================================================
--- /dev/null
+++
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,36 @@
+struct B {
+ int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {
+ // Introduce a TypedefNameDecl
+ using Iterator = D *;
+};
+
+struct C {
+ // This will cause use to invoke VisitTypedefNameDecl(...) when Importing
+ // the DeclContext of C.
+ // We will invoke ImportContext(...) which should force the From Decl to
+ // be defined if it not already defined. We will then Import the definition
+ // to the To Decl.
+ // This will force processing of the base class of D which allows us to see
+ // base class methods such as dump().
+ D::Iterator iter;
+
+ bool f(D *DD) {
+ return true; //%self.expect_expr("DD->dump()", result_type="int",
+ // result_value="42")
+ }
+};
+
+int main() {
+ C c;
+ D d;
+
+ c.f(&d);
+
+ return 0;
+}
Index:
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
===================================================================
--- /dev/null
+++
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: clang/lib/AST/ASTImporter.cpp
===================================================================
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -8166,7 +8166,19 @@
FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
return std::move(Err);
} else {
- CompleteDecl(ToRecord);
+ // If FromRecord is not defined we need to force it to be.
+ // Simply calling CompleteDecl(...) for a RecordDecl will break some
cases
+ // it will start the definition but we never finish it.
+ // If a RecordDecl has base classes they won't be imported and we will
+ // be missing anything that we inherit from those bases.
+ if (FromRecord->hasExternalLexicalStorage() &&
+ !FromRecord->isCompleteDefinition())
+ FromRecord->getASTContext().getExternalSource()->CompleteType(
+ FromRecord);
+
+ if (Error Err = ASTNodeImporter(*this).ImportDefinition(
+ FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
+ return std::move(Err);
}
} else if (auto *ToEnum = dyn_cast<EnumDecl>(ToDC)) {
auto *FromEnum = cast<EnumDecl>(FromDC);
Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===================================================================
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,36 @@
+struct B {
+ int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {
+ // Introduce a TypedefNameDecl
+ using Iterator = D *;
+};
+
+struct C {
+ // This will cause use to invoke VisitTypedefNameDecl(...) when Importing
+ // the DeclContext of C.
+ // We will invoke ImportContext(...) which should force the From Decl to
+ // be defined if it not already defined. We will then Import the definition
+ // to the To Decl.
+ // This will force processing of the base class of D which allows us to see
+ // base class methods such as dump().
+ D::Iterator iter;
+
+ bool f(D *DD) {
+ return true; //%self.expect_expr("DD->dump()", result_type="int",
+ // result_value="42")
+ }
+};
+
+int main() {
+ C c;
+ D d;
+
+ c.f(&d);
+
+ return 0;
+}
Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
===================================================================
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: clang/lib/AST/ASTImporter.cpp
===================================================================
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -8166,7 +8166,19 @@
FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
return std::move(Err);
} else {
- CompleteDecl(ToRecord);
+ // If FromRecord is not defined we need to force it to be.
+ // Simply calling CompleteDecl(...) for a RecordDecl will break some cases
+ // it will start the definition but we never finish it.
+ // If a RecordDecl has base classes they won't be imported and we will
+ // be missing anything that we inherit from those bases.
+ if (FromRecord->hasExternalLexicalStorage() &&
+ !FromRecord->isCompleteDefinition())
+ FromRecord->getASTContext().getExternalSource()->CompleteType(
+ FromRecord);
+
+ if (Error Err = ASTNodeImporter(*this).ImportDefinition(
+ FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
+ return std::move(Err);
}
} else if (auto *ToEnum = dyn_cast<EnumDecl>(ToDC)) {
auto *FromEnum = cast<EnumDecl>(FromDC);
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
