[lldb] Complete return types of CXXMethodDecls to prevent crashing due to covariant return types

[teemperor]

Summary

Currently we crash in Clang's CodeGen when we call functions with covariant return types with this assert:

Assertion failed: (DD && "queried property of class with no definition"), function data, file clang/include/clang/AST/DeclCXX.h, line 433.

when calling clang::CXXRecordDecl::isDerivedFrom from the ItaniumVTableBuilder.

Clang seems to assume that the underlying record decls of covariant return types are already completed.
This is true during a normal Clang invocation as there the type checker will complete both decls when
checking if the overloaded function is valid (i.e., the return types are covariant).

When we minimally import our AST into the expression in LLDB we don't do this type checking (which
would complete the record decls) and we end up trying to access the invalid record decls from CodeGen
which makes us trigger the assert.

This patch just completes the underlying types of ptr/ref return types of virtual function so that the
underlying records are complete and we behave as Clang expects us to do.

Fixes rdar://38048657

[teemperor]

Actually that seems to be more complicated in case we get the method decl by asking the ASTImporter for a definition (which will skip this code).

[teemperor]

• Made test stricter.
• Moved code to ASTImporterDelegate.

[shafik]

LGTM

lldb/source/Symbol/ClangASTImporter.cpp

1006 ↗

(On Diff #240156)

I am curious if we foresee the need to do this for other cases.

lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/Makefile

+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py

+import lldb
+from lldbsuite.test.lldbtest import *
+import lldbsuite.test.lldbutil as lldbutil
+
+class TestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def test(self):
+        self.build()
+        lldbutil.run_to_source_breakpoint(self,"// break here", lldb.SBFileSpec("main.cpp"))
+
+        # Test covariant return types for pointers to class that contains the called function.
+        self.expect_expr("derived.getPtr()", result_type="Derived *")
+        self.expect_expr("base_ptr_to_derived->getPtr()", result_type="Base *")
+        self.expect_expr("base.getPtr()", result_type="Base *")
+        # The same tests with reference types. LLDB drops the reference when it turns the
+        # result into a SBValue so check for the the underlying type of the result.
+        self.expect_expr("derived.getRef()", result_type="Derived")
+        self.expect_expr("base_ptr_to_derived->getRef()", result_type="Base")
+        self.expect_expr("base.getRef()", result_type="Base")
+
+        # Test covariant return types for pointers to class that does *not* contain the called function.
+        self.expect_expr("derived.getOtherPtr()", result_type="OtherDerived *")
+        self.expect_expr("base_ptr_to_derived->getOtherPtr()", result_type="OtherBase *")
+        self.expect_expr("base.getOtherPtr()", result_type="OtherBase *")
+        # The same tests with reference types. LLDB drops the reference when it turns the
+        # result into a SBValue so check for the the underlying type of the result.
+        self.expect_expr("derived.getOtherRef()", result_type="OtherDerived")
+        self.expect_expr("base_ptr_to_derived->getOtherRef()", result_type="OtherBase")
+        self.expect_expr("base.getOtherRef()", result_type="OtherBase")
+
+        # Test that we call the right function and get the right value back.
+        self.expect_expr("derived.getOtherPtr()->value()", result_summary='"derived"')
+        self.expect_expr("base_ptr_to_derived->getOtherPtr()->value()", result_summary='"derived"')
+        self.expect_expr("base.getOtherPtr()->value()", result_summary='"base"')
+        self.expect_expr("derived.getOtherRef().value()", result_summary='"derived"')
+        self.expect_expr("base_ptr_to_derived->getOtherRef().value()", result_summary='"derived"')
+        self.expect_expr("base.getOtherRef().value()", result_summary='"base"')

lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/main.cpp

+struct OtherBase {
+  // Allow checking actual type from the test by giving
+  // this class and the subclass unique values here.
+  virtual const char *value() { return "base"; }
+};
+struct OtherDerived : public OtherBase {
+  const char *value() override { return "derived"; }
+};
+
+struct Base {
+  OtherBase other_base;
+  // Function with covariant return type that is same class.
+  virtual Base* getPtr() { return this; }
+  virtual Base& getRef() { return *this; }
+  // Function with covariant return type that is a different class.
+  virtual OtherBase* getOtherPtr() { return &other_base; }
+  virtual OtherBase& getOtherRef() { return other_base; }
+};
+
+struct Derived : public Base {
+  OtherDerived other_derived;
+  Derived* getPtr() override { return this; }
+  Derived& getRef() override { return *this; }
+  OtherDerived* getOtherPtr() override { return &other_derived; }
+  OtherDerived& getOtherRef() override { return other_derived; }
+};
+
+int main() {
+  Derived derived;
+  Base base;
+  Base *base_ptr_to_derived = &derived;
+  (void)base_ptr_to_derived->getPtr();
+  (void)base_ptr_to_derived->getRef();
+  (void)base_ptr_to_derived->getOtherPtr();
+  (void)base_ptr_to_derived->getOtherRef();
+  return 0; // break here
+}

lldb/packages/Python/lldbsuite/test/lldbtest.py

def expect_expr(
             self.assertEqual(error_msg, eval_result.GetError().GetCString())
             return
 
         if not eval_result.GetError().Success():
             self.assertTrue(eval_result.GetError().Success(),
                 "Unexpected failure with msg: " + eval_result.GetError().GetCString())
 
         if result_type:
-            self.assertEqual(result_type, eval_result.GetTypeName())
+            self.assertEqual(result_type, eval_result.GetTypeName(), "Error when evaluating expr: " + expr)
 
         if result_value:
             self.assertEqual(result_value, eval_result.GetValue())
 
         if result_summary:
             self.assertEqual(result_summary, eval_result.GetSummary())
 
     def invoke(self, obj, name, trace=False):

lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

if (predicate(decl->getKind())) {
       if (!copied_decl)
         continue;
 
       if (FieldDecl *copied_field = dyn_cast<FieldDecl>(copied_decl)) {
         QualType copied_field_type = copied_field->getType();
 
         m_ast_importer_sp->RequireCompleteType(copied_field_type);
       }
+      // The member might be a function with a covariant return type. In this
+      // case Clang's CodeGen expects that the underlying record of the
+      // reference/pointer is complete. This is true when parsing C++ code as
+      // there the type checking will complete those classes to verify that
+      // the user provided actually covariant return types. But when we import
+      // our own AST in LLDB we have to simulate that behavior and also
+      // complete the underlying type of the result type.
+      if (CXXMethodDecl *method_decl = dyn_cast<CXXMethodDecl>(copied_decl)) {
+        QualType return_type = method_decl->getReturnType();
+
+        if (return_type->isReferenceType())
+          m_ast_importer_sp->RequireCompleteType(
+              return_type.getNonReferenceType());
+        if (return_type->isPointerType())
+          m_ast_importer_sp->RequireCompleteType(return_type->getPointeeType());
+      }
     } else {
       SkippedDecls = true;
     }
   }
 
   // CopyDecl may build a lookup table which may set up ExternalLexicalStorage
   // to false.  However, since we skipped some of the external Decls we must
   // set it back!