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

[aprantl]

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

[vsk]

Looks reasonable to me.

clang/lib/CodeGen/CGDebugInfo.cpp

3457

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

[rjmccall]

clang/lib/CodeGen/CGDebugInfo.h

417

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

[aprantl]

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]

clang/lib/CodeGen/CGDebugInfo.h

417

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

[aprantl]

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?

[rjmccall]

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]

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.

[rjmccall]

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.

[aprantl]

In D66121#1656442, @rjmccall wrote:

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.

[rjmccall]

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.

[aprantl]

This comment was removed by aprantl.

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

[rjmccall]

In D66121#1673233, @aprantl wrote:

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]

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

[aprantl]

Address comments from Vedant.

[vsk]

Thanks, lgtm.

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

26

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

clang/lib/CodeGen/CGDebugInfo.h

public:
   /// Emit a call to llvm.dbg.function.start to indicate
   /// start of a new function.
   /// \param Loc       The location of the function header.
   /// \param ScopeLoc  The location of the function body.
   void EmitFunctionStart(GlobalDecl GD, SourceLocation Loc,
                          SourceLocation ScopeLoc, QualType FnType,
                          llvm::Function *Fn, bool CurFnIsThunk,
                          CGBuilderTy &Builder);
 
   /// Start a new scope for an inlined function.
   void EmitInlineFunctionStart(CGBuilderTy &Builder, GlobalDecl GD);
   /// End an inlined function scope.
   void EmitInlineFunctionEnd(CGBuilderTy &Builder);
 
   /// Emit debug info for a function declaration.
   /// \p Fn is set only when a declaration for a debug call site gets created.
   void EmitFunctionDecl(GlobalDecl GD, SourceLocation Loc,
private:
   /// Retrieve the DIDescriptor, if any, for the canonical form of this
   /// declaration.
   llvm::DINode *getDeclarationOrDefinition(const Decl *D);
 
   /// \return debug info descriptor to describe method
   /// declaration for the given method definition.
   llvm::DISubprogram *getFunctionDeclaration(const Decl *D);
 
+  /// \return          debug info descriptor to the describe method declaration
+  ///                  for the given method definition.
+  /// \param FnType    For Objective-C methods, their type.
+  /// \param LineNo    The declaration's line number.
+  /// \param Flags     The DIFlags for the method declaration.
+  /// \param SPFlags   The subprogram-spcific flags for the method declaration.
+  llvm::DISubprogram *
+  getObjCMethodDeclaration(const Decl *D, llvm::DISubroutineType *FnType,
+                           unsigned LineNo, llvm::DINode::DIFlags Flags,
+                           llvm::DISubprogram::DISPFlags SPFlags);
+
   /// \return debug info descriptor to describe in-class static data
   /// member declaration for the given out-of-class definition.  If D
   /// is an out-of-class definition of a static data member of a
   /// class, find its corresponding in-class declaration.
   llvm::DIDerivedType *
   getOrCreateStaticDataMemberDeclarationOrNull(const VarDecl *D);
 
   /// Helper that either creates a forward declaration or a stub.

clang/lib/CodeGen/CGDebugInfo.cpp

     llvm::MDNode *PropertyNode = nullptr;
     if (ObjCImplementationDecl *ImpD = ID->getImplementation()) {
       if (ObjCPropertyImplDecl *PImpD =
               ImpD->FindPropertyImplIvarDecl(Field->getIdentifier())) {
         if (ObjCPropertyDecl *PD = PImpD->getPropertyDecl()) {
           SourceLocation Loc = PD->getLocation();
           llvm::DIFile *PUnit = getOrCreateFile(Loc);
           unsigned PLine = getLineNumber(Loc);
-          ObjCMethodDecl *Getter = PD->getGetterMethodDecl();
-          ObjCMethodDecl *Setter = PD->getSetterMethodDecl();
+          ObjCMethodDecl *Getter = PImpD->getGetterMethodDecl();
+          ObjCMethodDecl *Setter = PImpD->getSetterMethodDecl();
           PropertyNode = DBuilder.createObjCProperty(
               PD->getName(), PUnit, PLine,
               hasDefaultGetterName(PD, Getter)
                   ? ""
                   : getSelectorName(PD->getGetterName()),
               hasDefaultSetterName(PD, Setter)
                   ? ""
                   : getSelectorName(PD->getSetterName()),

   // merged with a potential upcoming definition.
   if (const auto *FD = dyn_cast<FunctionDecl>(D))
     return getFunctionForwardDeclaration(FD);
   else if (const auto *VD = dyn_cast<VarDecl>(D))
     return getGlobalVariableForwardDeclaration(VD);
 
   return nullptr;
 }
 
 llvm::DISubprogram *CGDebugInfo::getFunctionDeclaration(const Decl *D) {
   if (!D || DebugKind <= codegenoptions::DebugLineTablesOnly)
     return nullptr;
 
   const auto *FD = dyn_cast<FunctionDecl>(D);
   if (!FD)
     return nullptr;
 

       auto *SP = dyn_cast_or_null<llvm::DISubprogram>(MI->second);
       if (SP && !SP->isDefinition())
         return SP;
     }
   }
   return nullptr;
 }
 
+llvm::DISubprogram *CGDebugInfo::getObjCMethodDeclaration(
+    const Decl *D, llvm::DISubroutineType *FnType, unsigned LineNo,
+    llvm::DINode::DIFlags Flags, llvm::DISubprogram::DISPFlags SPFlags) {
+  if (!D || DebugKind <= codegenoptions::DebugLineTablesOnly)
+    return nullptr;
+
+  if (CGM.getCodeGenOpts().DwarfVersion < 5)
+    return nullptr;
+
+  // Starting with DWARF V5 method declarations are emitted as children of
+  // the interface type.
+  const auto *OMD = dyn_cast<ObjCMethodDecl>(D);
+  if (!OMD)
+    return nullptr;
+  auto *ID = dyn_cast_or_null<ObjCInterfaceDecl>(D->getDeclContext());
+  if (!ID)
+    ID = OMD->getClassInterface();
+  if (!ID)
+    return nullptr;
+  QualType QTy(ID->getTypeForDecl(), 0);
+  auto It = TypeCache.find(QTy.getAsOpaquePtr());
+  if (It == TypeCache.end())
+    return nullptr;
+  auto *InterfaceType = cast<llvm::DICompositeType>(It->second);
+  llvm::DISubprogram *FD = DBuilder.createFunction(
+      InterfaceType, getObjCMethodName(OMD), StringRef(),
+      InterfaceType->getFile(), LineNo, FnType, LineNo, Flags, SPFlags);
+  DBuilder.finalizeSubprogram(FD);
+  ObjCMethodCache[ID].push_back(FD);
+  return FD;
+}
+
 // getOrCreateFunctionType - Construct type. If it is a c++ method, include
 // implicit parameter "this".
 llvm::DISubroutineType *CGDebugInfo::getOrCreateFunctionType(const Decl *D,
                                                              QualType FnType,
                                                              llvm::DIFile *F) {
   if (!D || DebugKind <= codegenoptions::DebugLineTablesOnly)
     // Create fake but valid subroutine type. Otherwise -verify would fail, and
     // subprogram DIE will miss DW_AT_decl_file and DW_AT_decl_line fields.

     SPFlags |= llvm::DISubprogram::SPFlagOptimized;
 
   llvm::DINode::DIFlags FlagsForDef = Flags | getCallSiteRelatedAttrs();
   llvm::DISubprogram::DISPFlags SPFlagsForDef =
       SPFlags | llvm::DISubprogram::SPFlagDefinition;
 
   unsigned LineNo = getLineNumber(Loc);
   unsigned ScopeLine = getLineNumber(ScopeLoc);
+  llvm::DISubroutineType *DIFnType = getOrCreateFunctionType(D, FnType, Unit);
+  llvm::DISubprogram *Decl = nullptr;
+  if (D)
+    Decl = isa<ObjCMethodDecl>(D)
+               ? getObjCMethodDeclaration(D, DIFnType, LineNo, Flags, SPFlags)
+               : getFunctionDeclaration(D);
 
   // FIXME: The function declaration we're constructing here is mostly reusing
   // declarations from CXXMethodDecl and not constructing new ones for arbitrary
   // FunctionDecls. When/if we fix this we can have FDContext be TheCU/null for
   // all subprograms instead of the actual context since subprogram definitions
   // are emitted as CU level entities by the backend.
   llvm::DISubprogram *SP = DBuilder.createFunction(
-      FDContext, Name, LinkageName, Unit, LineNo,
-      getOrCreateFunctionType(D, FnType, Unit), ScopeLine, FlagsForDef,
-      SPFlagsForDef, TParamsArray.get(), getFunctionDeclaration(D));
+      FDContext, Name, LinkageName, Unit, LineNo, DIFnType, ScopeLine,
+      FlagsForDef, SPFlagsForDef, TParamsArray.get(), Decl);
   Fn->setSubprogram(SP);
   // We might get here with a VarDecl in the case we're generating
   // code for the initialization of globals. Do not record these decls
   // as they will overwrite the actual VarDecl Decl in the cache.
   if (HasDecl && isa<FunctionDecl>(D))
     DeclCache[D->getCanonicalDecl()].reset(SP);
 
   // We use the SPDefCache only in the case when the debug entry values option
   // is set, in order to speed up parameters modification analysis.
   //
   // FIXME: Use AbstractCallee here to support ObjCMethodDecl.
   if (CGM.getCodeGenOpts().EnableDebugEntryValues && HasDecl)
     if (auto *FD = dyn_cast<FunctionDecl>(D))
       if (FD->hasBody() && !FD->param_empty())
         SPDefCache[FD].reset(SP);
 
-  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
-    // Starting with DWARF V5 method declarations are emitted as children of
-    // the interface type.
-    if (const auto *OMD = dyn_cast_or_null<ObjCMethodDecl>(D)) {
-      const ObjCInterfaceDecl *ID = OMD->getClassInterface();
-      QualType QTy(ID->getTypeForDecl(), 0);
-      auto It = TypeCache.find(QTy.getAsOpaquePtr());
-      if (It != TypeCache.end()) {
-        llvm::DICompositeType *InterfaceDecl =
-            cast<llvm::DICompositeType>(It->second);
-        llvm::DISubprogram *FD = DBuilder.createFunction(
-            InterfaceDecl, Name, LinkageName, Unit, LineNo,
-            getOrCreateFunctionType(D, FnType, Unit), ScopeLine, Flags, SPFlags,
-            TParamsArray.get());
-        DBuilder.finalizeSubprogram(FD);
-        ObjCMethodCache[ID].push_back(FD);
-      }
-    }
-  }
-
   // Push the function onto the lexical block stack.
   LexicalBlockStack.emplace_back(SP);
 
   if (HasDecl)
     RegionMap[D].reset(SP);
 }
 
 void CGDebugInfo::EmitFunctionDecl(GlobalDecl GD, SourceLocation Loc,

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

+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone -dwarf-version=5 %s -o - | FileCheck %s
+
+@protocol NSObject
+@end
+
+@interface NSObject <NSObject> {}
+@end
+
+struct Bar {};
+
+@protocol BarProto
+@property struct Bar *bar;
+@end
+
+@interface Foo <BarProto>
+@end
+
+@implementation Foo {}
+@synthesize bar = _bar;
+- (void)f {}
+@end
+
+// CHECK: ![[FOO:[0-9]+]] = !DICompositeType(tag: DW_TAG_structure_type, name: "Foo"
+
+// CHECK: ![[DECL:[0-9]+]] = !DISubprogram(name: "-[Foo setBar:]",
+// CHECK-SAME:  scope: ![[FOO]]
+
+// CHECK: distinct !DISubprogram(name: "-[Foo setBar:]",
+// CHECK-SAME:  declaration: ![[DECL:[0-9]+]]