This is an archive of the discontinued LLVM Phabricator instance.

Fix crash due to ObjCPropertyDecl
ClosedPublic

Authored by dgoldman on Jan 18 2019, 8:46 AM.

Details

Summary

With ObjCPropertyDecl, ASTNode.OrigD can be a ObjCPropertyImplDecl
which is not a NamedDecl, leading to a crash since the code
incorrectly assumes ASTNode.OrigD will always be a NamedDecl.

Event Timeline

dgoldman created this revision.Jan 18 2019, 8:46 AM
ilya-biryukov accepted this revision.Jan 18 2019, 10:42 AM

Thanks for fixing this.
Quick LGTM to fix a crash, albeit a fews NITs.

clangd/index/SymbolCollector.cpp
372

Could you please leave a FIXME mentioning objc properties are not indexed properly here?

374

Maybe use llvm_dyn_cast and check for null? (To avoid double-cast)

This revision is now accepted and ready to land.Jan 18 2019, 10:42 AM
dgoldman updated this revision to Diff 182562.Jan 18 2019, 10:57 AM
dgoldman marked an inline comment as done.
  • FIXME and dyn_cast
dgoldman marked an inline comment as done.Jan 18 2019, 10:58 AM

@dgoldman, do you have commit access or should I land this for you?

clangd/index/SymbolCollector.cpp
379

NIT: use auto*, the type name is spelled in the RHS anyway.

Test case? Or does one already cover this crash?

Good point, @aaron.ballman! @dgoldman, could you please add a test case?

dgoldman updated this revision to Diff 182921.Jan 22 2019, 8:07 AM
dgoldman marked an inline comment as done.
  • use auto

Good point, @aaron.ballman! @dgoldman, could you please add a test case?

Unfortunately I can't get ninja check-clangd to build:

/Users/davg/dev/llvm/source/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:54:27: error: no matching member function for call to 'getThisType'
        ActualMemberDecl->getThisType(ActualMemberDecl->getASTContext())
        ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
/Users/davg/dev/llvm/source/llvm/tools/clang/include/clang/AST/DeclCXX.h:2182:12: note: candidate function not viable: requires 0 arguments, but 1 was provided
  QualType getThisType() const;
           ^
/Users/davg/dev/llvm/source/llvm/tools/clang/include/clang/AST/DeclCXX.h:2184:19: note: candidate function not viable: requires 2 arguments, but 1 was provided
  static QualType getThisType(const FunctionProtoType *FPT,
                  ^
1 error generated.
[1230/1512] Building CXX object tools/clang/tools/extra/clang-tidy/cert/CMakeFiles/clangTidyCERTModule.dir/CERTTidyModule.cpp.o
ninja: build stopped: subcommand failed.

Unfortunately I can't get ninja check-clangd to build:

Looks like clang-tools-extra uses an old revision. Rebasing after rL350916 should do the trick.

dgoldman updated this revision to Diff 182942.Jan 22 2019, 10:27 AM
  • FIXME and dyn_cast
  • use auto
  • Add test

Unfortunately I can't get ninja check-clangd to build:

Looks like clang-tools-extra uses an old revision. Rebasing after rL350916 should do the trick.

Thanks, looks like my update script was skipping over that repo. Test case implemented. As I don't have commit access, can you land this for me?

This revision was automatically updated to reflect the committed changes.

Done. I had to remove Container::_magic from the list of expeced symbols to make the test pass, let me know if it should actually be there.
Again, thanks for the fix!

It looks like Container::_magic is a platform-dependent completion, I don't have it on Linux, but http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/42665 fails because it's not in the list.
Submitted rL351943 to workaround the failure. @dgoldman, any idea why completion might be there on some platforms, but not the others?

It looks like Container::_magic is a platform-dependent completion, I don't have it on Linux, but http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/42665 fails because it's not in the list.
Submitted rL351943 to workaround the failure. @dgoldman, any idea why completion might be there on some platforms, but not the others?

Perhaps property auto-synthesis isn't enabled there? I wonder if the following manual synthesis would help:

@implementation Container
@synthesize magic;
@end