Don't crash if a member declaration is incomplete

[loladiro]

Summary

This is progress toward PR19676/PR20781 and should have both cases error rather than crashing the debugger
(The former PR already had a similar work around, but I believe the original test case had the same issue as PR20781, so it never fully worked). Ideally, we need to figure out how to search other images in the process for a definition of this class at this point, instead of just pretending it's empty (which is what this patch does), but I don't know enough about LLDB to make that happen. Pointers appreciated.

[emaste]

FYI if uploading diffs through the web interface it's useful to generate them with full context - e.g. git diff -U99999 or svn diff --diff-cmd=diff -x -U999999

[clayborg]

Changes to ClangASTType::StartTagDeclarationDefinition() should not be needed. Please verify that the CXXRecordDecl is handled by the TagDecl:

const clang::Type *t = qual_type.getTypePtr();
if (t)
{
    const clang::TagType *tag_type = llvm::dyn_cast<clang::TagType>(t);
    if (tag_type)
    {
        clang::TagDecl *tag_decl = tag_type->getDecl();
        if (tag_decl)
        {
            tag_decl->startDefinition();
            return true;
        }
    }

source/Symbol/ClangASTType.cpp

5851–5857

This patch is not needed. CXXRecordDecl is a TagType and it will be handled by the code below.

[dblaikie]

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

2182

Is this the same error text (modulo "member" rather than "inheritance") as the inheritance case that was already handled? Should the message go in one place rather than being duplicated?

source/Symbol/ClangASTType.cpp

5851

I'm guessing this (without context (both in the patch, and as I'm not an LLDB developer) I'm stabbing in the dark) is to address the forward decl as a base class produced by templates case? That wasn't handled, judging by the test case you added (or was that test case just for coverage, even though it was already covered?)?

test/types/TestForwardTypes.py

34

Should these tests test for the specific error message/behavior rather than "do anything so long as it's not crashing"?

test/types/forward_inheritance.cpp

3

(I'd usually use struct rather than class when access doesn't matter - but... it doesn't matter, so whichever suits)

10

you could drop the 'public' here, it doesn't matter (though, again, would probably use struct rather than class, as well)

test/types/forward_member.cpp

8

are the typedefs (of foo<int> and of bar) required here? I imagine not (similarly for the forward_inheritance example)

[loladiro]

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

2182

It's a similar error message but it's in a different place (because of where the bases vs members are constructed. I don't think it's worth moving the error message until we have a way to actually search the rest of the images).

source/Symbol/ClangASTType.cpp

5851

This is required for both cases. The code was there, but it doesn't seem like it ever actually worked because of this.

5851–5857

I thought so too, but it crashed without it. I now, realize that all that's required is to use getAsTagDecl below rather than casting through the TagType. That should handle this case as well. Will update.

test/types/TestForwardTypes.py

34

Yes, but I didn't know how to do that, so I figured I'd put it up and somebody who know lldb will help out.

test/types/forward_member.cpp

8

For some reason I had trouble getting it to crash without the typedefs, but that was before I understood the issue fully, so I'll have another go at it.

[loladiro]

Updated to use getAsTagDecl.

[clayborg]

Looks good.

[loladiro]

@clayborg, would you mind trying out the tests and seeing if they can be improved. I've tried getting the LLDB tests running, but on Linux the tests just generally fail for me and on OS X, LLDB python segfaults as soon as it tries to load LLDB, so I wasn't able to actually run them. Also, I don't have any experience writing LLDB tests, so I'm sure they need to be improved.

[vharron]

Hi Keno,

The tests are generally passing cleanly on Linux and OSX for us. Can you
post complete repro steps so we might be able to help troubleshoot?

Please try building with CMake on Linux and Xcode on OSX to see if this
improves for you.

Vince

[loladiro]

Hi @vharron,

both my configurations were fresh clones of LLVM/Clang/LLDB,
On Linux using CMake+Ninja, I got the following output from ninja check-lldb, https://gist.github.com/Keno/7a145dd774aead9db73d
On OS X using CMake+Ninja, ninja check opens up a crash dialog for python, indicating a null pointer exception when importing the lldb module. I have not tested with XCode on OS X, though I can if you would like.

[vharron]

The test suite appears to be failing because it's failing to find a suitable compiler. Try to

If you're on Ubuntu 14.04

sudo apt-get install libc++-dev:amd64 libc++-dev:i386 clang-3.5

And try again.

[loladiro]

Didn't help:

kfischer@julia:~/llvm35/llvm-build$ dpkg --get-selections | grep clang
clang						install
clang-3.6					install
libclang-common-3.6-dev				install
libclang1-3.6:amd64				install
kfischer@julia:~/llvm35/llvm-build$ dpkg --get-selections | grep libc++
libc++-dev:amd64				install
libc++-dev:i386					install
libc++-helpers					install
libc++1:amd64					install
libc++1:i386					install

Do I need to reconfigure something?

[vharron]

Ah, if clang-3.6 is on your path try -C clang-3.6 instead of /usr/bin/cc

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

for (die = parent_die->GetFirstChild(); die != NULL; die = die->GetSibling())
                                                                                            encoding_uid,
                                                                                            MakeUserID(parent_die->GetOffset()));
                                             }
                                             
                                             member_clang_type = GetClangASTContext().CreateArrayType(member_array_element_type, 0, false);
                                         }
                                     }
                                 }
-                                
+                                if (member_clang_type.GetCompleteType() == false)
+                                {
+                                    GetObjectFile()->GetModule()->ReportError ("0x%8.8" PRIx64 "  DW_TAG_member '%s' refers to type '%s' that is a forward declaration, not a complete definition."
+                                                                               "\nThis can happen due to missing images or compiler bugs.",
+                                                                               MakeUserID(die->GetOffset()),
+                                                                               name,
+                                                                               member_clang_type.GetTypeName().GetCString());
+
+                                    // We have no choice other than to pretend that the base class
+                                    // is complete. If we don't do this, clang will crash later when computing
+                                    // the object layout.
+                                    member_clang_type.StartTagDeclarationDefinition ();
+                                    member_clang_type.CompleteTagDeclarationDefinition ();
+                                }
+
+
                                 field_decl = class_clang_type.AddFieldToRecordType (name,
                                                                                     member_clang_type,
                                                                                     accessibility,
                                                                                     bit_size);
                                 
                                 GetClangASTContext().SetMetadataAsUserID (field_decl, MakeUserID(die->GetOffset()));
 
                                 layout_info.field_offsets.insert(std::make_pair(field_decl, field_bit_offset));

source/Symbol/ClangASTType.cpp

 #pragma mark TagDecl
 
 bool
 ClangASTType::StartTagDeclarationDefinition ()
 {
     if (IsValid())
     {
         clang::QualType qual_type (GetQualType());
         const clang::Type *t = qual_type.getTypePtr();
         if (t)
         {
-            const clang::TagType *tag_type = llvm::dyn_cast<clang::TagType>(t);
-            if (tag_type)
-            {
-                clang::TagDecl *tag_decl = tag_type->getDecl();
-                if (tag_decl)
-                {
-                    tag_decl->startDefinition();
-                    return true;
-                }
-            }
+            clang::TagDecl *tag_decl = t->getAsTagDecl();
             
             const clang::ObjCObjectType *object_type = llvm::dyn_cast<clang::ObjCObjectType>(t);
             if (object_type)
             {
                 clang::ObjCInterfaceDecl *interface_decl = object_type->getInterface();
                 if (interface_decl)
                 {
                     interface_decl->startDefinition();

test/types/TestForwardTypes.py

+"""
+Test that forward declarations to types in other compilation units do not crash
+the debugger.
+"""
+
+import AbstractBase
+import unittest2
+import lldb
+import sys
+from lldbtest import *
+
+class ForwardTypesTestCase(AbstractBase.GenericTester):
+    mydir = AbstractBase.GenericTester.compute_mydir(__file__)
+
+    def setUp(self):
+        # Call super's setUp().
+        AbstractBase.GenericTester.setUp(self)
+        # disable "There is a running process, kill it and restart?" prompt
+        self.runCmd("settings set auto-confirm true")
+        self.addTearDownHook(lambda: self.runCmd("settings clear auto-confirm"))
+
+    @dwarf_test
+    def test_forward_member(self):
+        """Test that members with forwarded debug info don't crash the compiler"""
+        self.source = 'forward_member.cpp'
+        self.buildDwarf()
+        target = self.dbg.createTarget("a.out")
+        self.assertTrue(target, VALID_TARGET)
+        self.runCmd("p (bar_t*)0")
+
+    @dwarf_test
+    def test_forward_inheritance(self):
+        """Test that bases with forwarded debug info don't crash the compiler"""
+        self.source = 'forward_inheritance.cpp'
+        self.buildDwarf()
+        target = self.dbg.createTarget("a.out")
+        self.assertTrue(target, VALID_TARGET)
+        self.runCmd("p (bar*)0")
+
+if __name__ == '__main__':
+    import atexit
+    lldb.SBDebugger.Initialize()
+    atexit.register(lambda: lldb.SBDebugger.Terminate())
+    unittest2.main()

test/types/forward_inheritance.cpp

+template<typename T>
+class foo {
+  void func() { }
+};
+ 
+extern template class foo<int>;
+typedef foo<int> fooint; 
+
+class bar : public fooint {
+};
+
+bar f;

test/types/forward_member.cpp

+template<typename T>
+struct foo {
+  void func() { }
+};
+ 
+extern template struct foo<int>;
+typedef foo<int> fooint; 
+
+typedef struct bar {
+    fooint foo;
+} bar_t;
+
+bar_t f;