This is an archive of the discontinued LLVM Phabricator instance.

Add Objective-C property accessors loaded from Clang module DWARF to lookup
ClosedPublic

Authored by aprantl on Apr 16 2020, 2:46 PM.

Details

Summary

This patch fixes a bug when synthesizing an ObjC property from -gmodules debug info. Because the method declaration that is injected via the non-modular property implementation is not added to the ObjCInterfaceDecl's lookup pointer, a second copy of the accessor would be generated when processing the ObjCPropertyDecl. This can be avoided by finding the existing method decl in ClangExternalASTSourceCallbacks::FindExternalVisibleDeclsByName() and adding it to the LookupPtr.

Diff Detail

Event Timeline

I'm not sure I understand why are we not adding the property to the lookup ptr? I would assume we would call addDecl to it which should make it visible and I don't see any ObjCPropertyDecl (?) check in that code?

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
348

SetMemberOwningModule is called really often and these two conditions are always met in our code, so we are now constantly resetting setHasExternalVisibleStorage to true?

351

You shouldn't need this to get FindExternalVisibleDeclsByName and all this seems to work without it?

I'm not sure I understand why are we not adding the property to the lookup ptr? I would assume we would call addDecl to it which should make it visible and I don't see any ObjCPropertyDecl (?) check in that code?

Thanks! This sounds like exactly the kind of feedback I was hoping for, just to be sure, could you please clarify what you mean specifically? The problem is that the ObjCMethodDecl for the getter that comes from the @synthesize declaration is missing in the LookupPtr and therefore this check:

clang::ObjCMethodDecl *getter = nullptr;
if (!getter_sel.isNull())
  getter = isInstance ? class_interface_decl->lookupInstanceMethod(getter_sel)
                      : class_interface_decl->lookupClassMethod(getter_sel);

fails and so TypeSystemClang::AddObjCClassProperty() synthesizes a second getter. Where are you suggesting to add the ObjCPropertyDecl to the lookup ptr and how would that help?

teemperor accepted this revision.Apr 24 2020, 9:04 AM

So it seems there aren't any alternative solutions that are less invasive, so I think this can go in.

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
348

Actually this probably can stay. It's not perfect that we keep resetting, but we should usually add all members at once I think, so it's not really making this thing more complicated.

351

So if you move this above the comment this is already good enough. It's not needed for FindExternalVisibleDeclsByName but it makes sense that this has external lexical storage.

This revision is now accepted and ready to land.Apr 24 2020, 9:04 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2020, 11:21 AM