[Lldb-commits] [PATCH] Don't crash if a member declaration is incomplete

[Keno Fischer]

REPOSITORY
  rL LLVM

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2182
@@ +2181,3 @@
+                                {
+                                    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.",
----------------
dblaikie wrote:
> 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?
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). 

================
Comment at: source/Symbol/ClangASTType.cpp:5851
@@ -5850,1 +5850,3 @@
         clang::QualType qual_type (GetQualType());
+        clang::CXXRecordDecl *cxx_record_decl = qual_type->getAsCXXRecordDecl();
+        if (cxx_record_decl)
----------------
dblaikie wrote:
> loladiro wrote:
> > clayborg wrote:
> > > This patch is not needed. CXXRecordDecl is a TagType and it will be handled by the code below. 
> > 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.
> 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?)?
This is required for both cases. The code was there, but it doesn't seem like it ever actually worked because of this.

================
Comment at: source/Symbol/ClangASTType.cpp:5851-5857
@@ -5850,2 +5850,9 @@
         clang::QualType qual_type (GetQualType());
+        clang::CXXRecordDecl *cxx_record_decl = qual_type->getAsCXXRecordDecl();
+        if (cxx_record_decl)
+        {
+            cxx_record_decl->startDefinition();
+            return true;
+        }
+
         const clang::Type *t = qual_type.getTypePtr();
----------------
clayborg wrote:
> This patch is not needed. CXXRecordDecl is a TagType and it will be handled by the code below. 
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.

================
Comment at: test/types/TestForwardTypes.py:33
@@ +32,3 @@
+    def test_forward_inheritance(self):
+        """Test that bases with forwarded debug info don't crash the compiler"""
+        self.source = 'forward_inheritance.cpp'
----------------
dblaikie wrote:
> Should these tests test for the specific error message/behavior rather than "do anything so long as it's not crashing"?
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.

================
Comment at: test/types/forward_member.cpp:7
@@ +6,3 @@
+extern template struct foo<int>;
+typedef foo<int> fooint; 
+
----------------
dblaikie wrote:
> are the typedefs (of foo<int> and of bar) required here? I imagine not (similarly for the forward_inheritance example)
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.

http://reviews.llvm.org/D10509

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/