Page MenuHomePhabricator

Debug Info: Nest Objective-C property function decls inside their container.
ClosedPublic

Authored by aprantl on Aug 12 2019, 4:44 PM.

Details

Summary

This fixes a crash in Clang.

Starting with DWARF 5 we are emitting ObjC method declarations as
children of their containing entity. This worked for interfaces, but
didn't consider the case of synthessized properties. When a property
of a protocol is synthesized in an interface implementation the
ObjCMethodDecl that was passed to CGF::StartFunction was the property
*declaration* which obviously couldn't have a containing
interface. This patch passes the containing interface all the way
through to CGDebugInfo, so the function declaration can be created
with the correct parent (= the class implementing the protocol).

rdar://problem/53782400

Diff Detail

Event Timeline

aprantl created this revision.Aug 12 2019, 4:44 PM
aprantl added a reviewer: davide.
vsk added a comment.Aug 12 2019, 4:54 PM

Looks reasonable to me.

clang/lib/CodeGen/CGDebugInfo.cpp
3505

Return early on '! dyn_cast<ObjCMethodDecl>'? It doesn't look like 'D' may be null.

rjmccall added inline comments.Aug 12 2019, 11:29 PM
clang/lib/CodeGen/CGDebugInfo.h
417

Why does this have to be an extra parameter? The method's DC should be the container.

aprantl marked an inline comment as done.Aug 13 2019, 10:48 AM
aprantl added inline comments.
clang/lib/CodeGen/CGDebugInfo.h
417
@protocol BarProto
@property struct Bar *bar;
@end

@interface Foo <BarProto>
@end

@implementation Foo {}
@synthesize bar = _bar;
@end

The ObjCMethodDecl for the synthesized bar accessors is the method decl in the protocol BarProto, as there is no method decl put into the AST inside of Foo. Its DeclContext (lexical an regular) therefor is the Protocol, which is not what we want as the decl context for a synthesized method called -[Foo setBar:] in the debug info.

rjmccall added inline comments.Aug 13 2019, 11:25 AM
clang/lib/CodeGen/CGDebugInfo.h
417

Wait, really? That seems pretty unfortunate. Should we synthesize a real implementation method?

aprantl marked an inline comment as done.Aug 13 2019, 1:51 PM
aprantl added inline comments.
clang/lib/CodeGen/CGDebugInfo.h
417

That certainly sounds like the more elegant solution. I didn't know enough about Clang's ObjC implementation to understand whether this was feasible. Do you happen to know where a good entry point for doing this would be?

vsk edited reviewers, added: ahatanak, erik.pilkington; removed: vsk.Aug 13 2019, 2:06 PM
rjmccall added inline comments.Aug 13 2019, 2:26 PM
clang/lib/CodeGen/CGDebugInfo.h
417

It might be a consequential change, but I think the right place would just be in Sema::ActOnPropertyImplDecl, near the bottom where it pulls out the getter and setter methods. You should be able to just create new implicit ObjCMethodDecls as redeclarations of the getter/setter and add them to the ObjCImplDecl. It might also be nice to link them off of the ObjCPropertyImplDecl, but that's not as necessary.

If you do that, you'll probably need to update IRGen so that we don't attempt to emit the synthesized methods twice.

aprantl updated this revision to Diff 218000.Aug 29 2019, 5:00 PM

Here is a work-in-progress alternative patch that attacks the problem by changing the AST generation to have an ObjCMethodDecl for the property accessors inside the implementation as suggested by @rjmccall.

The patch passes all clang-codegen-* tests, with a few problems in c-index-test outstanding that I'll work through.

Let me know if you have any feedback on this approach! I'd be curious if there is a more elegant way of doing this: The code that I needed to add to ObjCMethodDecl::getNextRedeclarationImpl() is a bit odd (see the example in the FIXME comment there). It's also weird that Objective-C methods can have exactly one redeclaration which forces me to set up a chain of redeclarations in Sema::ActOnPropertyImplDecl() instead of having several implementations point to the same abstract property declaration, but that seems to work.

Can you prepare an NFC patch with just the changes relating to adding ObjCPropertyImplDecl::get{Getter,Setter}MethodDecl?

I don't get why the redeclaration logic is changing. What happens when a normal method is implemented? Is that not linked up as a redeclaration?

clang/lib/CodeGen/CodeGenFunction.cpp
638 ↗(On Diff #218000)

Now-spurious style change.

Can you prepare an NFC patch with just the changes relating to adding ObjCPropertyImplDecl::get{Getter,Setter}MethodDecl?

Sure, I will do that.

I don't get why the redeclaration logic is changing. What happens when a normal method is implemented? Is that not linked up as a redeclaration?

From what I can tell from injecting an assertion into ObjCMethodDecl::setAsRedeclaration() only redeclarations within the same protocol/interface/category are linked up as redeclarations. An implementation of an interface is not a redeclaration in the SemaObjC sense. Digging further, this is implemented in https://github.com/llvm/llvm-project/blob/244e738485445fa4b72bfef9b9b2f9625cee989e/clang/lib/Sema/SemaDeclObjC.cpp#L3930 by checking whether a method's selector has been processed before within the same property/interface/category.

That's very different from what my patch is doing. The following example is extracted from clang/test/CodeGenObjC/newproperty-nested-synthesis-1.m:

 1	@interface Tester : Object
 2	@property char PropertyAtomic_char;
 3	@end
 4	
 5	@implementation Tester
 6	@dynamic PropertyAtomic_char;
 7	@end
 8	
 9	@interface SubClass : Tester
10	{
11	    char PropertyAtomic_char;
12	}
13	@end
14	
15	@implementation SubClass
16	@synthesize PropertyAtomic_char;
17	@end

My patch sets the getter synthesized at line 6 up as a redeclaration of getter from the interface on line 2 and the getter from SubClass on line 16 as a redeclaration of the getter from Tester on line 6. That must be wrong: If there were another subclass of Tester with another getter, the compiler would assert, because each Objective-C method declaration can have at most one redeclaration.

So in summary I think what I was trying to do doesn't make sense in the existing infrastructure and I shouldn't be trying to set up methods as redeclarations between *different* interfaces/protocols/implementations. Do you agree with this assessment? If yes I'll try to redo the patch without using the redeclaration mechanism.

Right, that sounds reasonable. You should be making them link up exactly like a normal method implementation, so if SemaObjC doesn't consider normal method implementations to be redeclarations, then I guess these aren't either. And if we want to change that in the future, then we'll change it for these, too.

This comment was removed by aprantl.

I'm afraid I'm going to give up on fixing the AST and return to my debuginfo-only patch.

While I was correct in figuring out that ObjCMethodDecl implementations are not linked up as redeclarations of ObjCMethodDecl decls in the interface, that wasn't the whole story: ObjCMethodDecl::getNextRedeclarationImpl() links them up *implicitly* by finding the implementation for a decl and vice versa by looking up the selector in the interface/implementation (See https://github.com/llvm/llvm-project/blob/244e738485445fa4b72bfef9b9b2f9625cee989e/clang/lib/AST/DeclObjC.cpp#L905).

I can make this mechanism work by adding the method to the ObjCImplementationDecl, so it gets found by getNextRedeclarationImpl(). But if I do that, CodeGen falls over completely, because the new property accessor redeclarations don't actually have any function bodies. CodeGen in several places special-cases property accessors to generate them on-the-fly. I think this may work neatly if we actually created an AST for the function body in SemaObjCProperty too, and remove the functionality from CodeGen, but that is beyond what I'm prepared to do.

I'm afraid I'm going to give up on fixing the AST and return to my debuginfo-only patch.

While I was correct in figuring out that ObjCMethodDecl implementations are not linked up as redeclarations of ObjCMethodDecl decls in the interface, that wasn't the whole story: ObjCMethodDecl::getNextRedeclarationImpl() links them up *implicitly* by finding the implementation for a decl and vice versa by looking up the selector in the interface/implementation (See https://github.com/llvm/llvm-project/blob/244e738485445fa4b72bfef9b9b2f9625cee989e/clang/lib/AST/DeclObjC.cpp#L905).

I can make this mechanism work by adding the method to the ObjCImplementationDecl, so it gets found by getNextRedeclarationImpl(). But if I do that, CodeGen falls over completely, because the new property accessor redeclarations don't actually have any function bodies. CodeGen in several places special-cases property accessors to generate them on-the-fly. I think this may work neatly if we actually created an AST for the function body in SemaObjCProperty too, and remove the functionality from CodeGen, but that is beyond what I'm prepared to do.

Can you just check for a method that doesn't have a body and use that as the trigger for emitting the synthetic getter/setter body, and get rid of the old IRGen code that looks for property implementations and emits them?

aprantl updated this revision to Diff 222048.Sep 26 2019, 4:53 PM

Factored out the general AST changes into https://reviews.llvm.org/D68108.

aprantl updated this revision to Diff 228484.Fri, Nov 8, 10:01 AM
aprantl marked 3 inline comments as done.
aprantl added a reviewer: vsk.

Address comments from Vedant.

aprantl marked an inline comment as done.Fri, Nov 8, 10:23 AM
vsk accepted this revision.Fri, Nov 8, 2:55 PM

Thanks, lgtm.

clang/test/CodeGenObjC/debug-info-objc-property-dwarf5.m
27

Should this check that the subprogram for 'getBar' is nested within Foo as well?

This revision is now accepted and ready to land.Fri, Nov 8, 2:55 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFri, Nov 8, 3:20 PM