r184473 - Debug Info: Attempt to resolve forward declarations if we are not emitting

[Adrian Prantl]

On Jun 20, 2013, at 2:32 PM, David Blaikie <dblaikie at gmail.com> wrote:

> On Thu, Jun 20, 2013 at 2:17 PM, Adrian Prantl <aprantl at apple.com> wrote:
>> Author: adrian
>> Date: Thu Jun 20 16:17:24 2013
>> New Revision: 184473
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=184473&view=rev
>> Log:
>> Debug Info: Attempt to resolve forward declarations if we are not emitting
>> limited debug info.
> 
> As discussed in Keith's thread ("[PATCH] Output debug information for
> structure members only referenced by a pointer or typedef") I have
> been looking into this (I suppose I could've assigned PR16214 to me)
> 
> Wouldn't've minded discussing this approach first, but either way will
> do I guess.

Sorry, I missed that one!

>> This is another small addendum to r184252.
>> 
>> rdar://problem/14101097
> 
> PR16214.
> 

Interesting!

> 
> Is recursion really the right tool here? Seems a bit gratuitous.

To me it feels more natural this way, but obviously we can rewrite is as a loop, sure.

> 
>> +  }
>> +
>> +  return false;
>> +}
>> +
>> /// getCompletedTypeOrNull - Get the type from the cache or return null if it
>> /// doesn't exist.
>> llvm::DIType CGDebugInfo::getCompletedTypeOrNull(QualType Ty) {
>> @@ -1904,8 +1920,17 @@ llvm::DIType CGDebugInfo::getCompletedTy
>>   }
>> 
>>   // Verify that any cached debug info still exists.
>> -  if (V != 0)
>> -    return llvm::DIType(cast<llvm::MDNode>(V));
>> +  if (V != 0) {
>> +    llvm::DIType CachedType(cast<llvm::MDNode>(V));
>> +
>> +    // Unless we are limiting debug info, attempt to resolve any
>> +    // forward declarations.
>> +    if (DebugKind > CodeGenOptions::LimitedDebugInfo &&
>> +        ContainsFwdDecl(CachedType))
>> +      return llvm::DIType();
> 
> So this doesn't account for declaration V definition - this means if
> we repeatedly need the declaration we'll keep building new
> declarations of the original type and its derived types. This "works
> out" because of metadata uniquing, but leaves me a bit uncomfortable.
agreed.
> 
> This also doesn't apply to pointers, either - we'll needlessly rebuild
> pointer types every time even though we could never want to emit the
> definition of the thing they point to, we only ever need the
> declaration.

By “if we only ever need the declaration”, do you mean "if there is no definition”? With -fno-limit-debug-info I would assume we _want_ everything.
> 
> I'd sort of rather we take a more explicit approach, but I've not
> figured out exactly what will work nicely yet.
> 

Yes, originally I planned to implement it like this, but since we are using the raw clang::QualType pointers as hash values for the type cache this does not work:

diff --git a/lib/CodeGen/CGDebugInfo.cpp b/lib/CodeGen/CGDebugInfo.cpp
index 563faa4..4be954f 100644
--- a/lib/CodeGen/CGDebugInfo.cpp
+++ b/lib/CodeGen/CGDebugInfo.cpp
@@ -1886,6 +1886,17 @@ llvm::DIType CGDebugInfo::getTypeOrNull(QualType Ty) {
   return llvm::DIType();
 }
 
+void CGDebugInfo::CompleteFwdDecls(const RecordDecl *RD) {
+  assert(RD);
+  if (DebugKind <= CodeGenOptions::LimitedDebugInfo)
+    return;
+
+  QualType QTy = CGM.getContext().getRecordType(RD);
+  llvm::DIType T = getCompletedTypeOrNull(QTy);
+
+  if (T.Verify() && T.isForwardDecl()) {
+    llvm::DIFile DefUnit = getOrCreateFile(RD->getLocation());
+    getOrCreateType(QTy, DefUnit);
+  }
+}
+
 /// getCachedInterfaceTypeOrNull - Get the type from the interface
 /// cache, unless it needs to regenerated. Otherwise return null.
 llvm::Value *CGDebugInfo::getCachedInterfaceTypeOrNull(QualType Ty) {
diff --git a/lib/CodeGen/CGDebugInfo.h b/lib/CodeGen/CGDebugInfo.h
index fc62600..5b9299a 100644
--- a/lib/CodeGen/CGDebugInfo.h
+++ b/lib/CodeGen/CGDebugInfo.h
@@ -289,6 +289,8 @@ public:
   llvm::DIType getOrCreateInterfaceType(QualType Ty,
                                         SourceLocation Loc);
 
+  void CompleteFwdDecls(const RecordDecl *TD);
+
 private:
   /// EmitDeclare - Emit call to llvm.dbg.declare for a variable declaration.
   void EmitDeclare(const VarDecl *decl, unsigned Tag, llvm::Value *AI,
diff --git a/lib/CodeGen/CodeGenTypes.cpp b/lib/CodeGen/CodeGenTypes.cpp
index 4240216..a2b13c2 100644
--- a/lib/CodeGen/CodeGenTypes.cpp
+++ b/lib/CodeGen/CodeGenTypes.cpp
@@ -260,6 +260,10 @@ void CodeGenTypes::UpdateCompletedType(const TagDecl *TD) {
   // yet, we'll just do it lazily.
   if (RecordDeclTypes.count(Context.getTagDeclType(RD).getTypePtr()))
     ConvertRecordDeclType(RD);
+
+  // Complete the debug info for this type if applicable.
+  if (CGDebugInfo *DI = CGM.getModuleDebugInfo())
+    DI->CompleteFwdDecls(RD);
 }
 
 static llvm::Type *getTypeForFormat(llvm::LLVMContext &VMContext,


cheers,
adrian