Page MenuHomePhabricator

Don't crash if a member declaration is incomplete
AcceptedPublic

Authored by loladiro on Jun 17 2015, 10:31 AM.

Details

Reviewers
clayborg
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.

Diff Detail

Repository
rL LLVM

Event Timeline

loladiro updated this revision to Diff 27845.Jun 17 2015, 10:31 AM
loladiro retitled this revision from to Don't crash if a member declaration is incomplete.
loladiro updated this object.
loladiro edited the test plan for this revision. (Show Details)
loladiro added a reviewer: clayborg.
loladiro set the repository for this revision to rL LLVM.
loladiro added subscribers: Unknown Object (MLST), dblaikie.
emaste added a subscriber: emaste.Jun 17 2015, 10:36 AM

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 requested changes to this revision.Jun 17 2015, 10:42 AM
clayborg edited edge metadata.

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.

This revision now requires changes to proceed.Jun 17 2015, 10:42 AM
dblaikie added inline comments.Jun 17 2015, 10:44 AM
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
33

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
2

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

9

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
7

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

loladiro added inline comments.Jun 17 2015, 10:59 AM
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
33

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
7

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 this revision to Diff 27848.Jun 17 2015, 11:01 AM
loladiro edited edge metadata.

Updated to use getAsTagDecl.

clayborg accepted this revision.Jun 17 2015, 1:04 PM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Jun 17 2015, 1:04 PM

@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.

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

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.

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.

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?

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