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

[David Blaikie]

On Thu, Jun 20, 2013 at 2:52 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
> 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.

Sure - the distinction between limited & unlimited debug info in this
regard should be contained wholely within the one condition (that you
fixed) in CGDebugInfo::CreateType(const RecordType*, bool) - outside
that one conditional no other code (or brain power :)) should be
concerned with whether we actually emit definitions or declarations -
just with what is 'needed'.

But regardless of the limited/unlimited debug info - my point still
stands. In both forms your solution will cause us to rebuild new
metadata nodes (that will be uniqued & not actually ever be
observable, except in performance) for pointer types even though we'll
never do anything different for them each time we emit them.

Though as I write this, perhaps I see what you mean - if we have code like:

struct foo;

void f1() {
  foo *f;
}

struct foo {
};

void f2() {
  foo *f;
}

Then in -fno-limit-debug-info we do want to have f2::f cause us to
emit the full definition of 'foo'.

>>
>> 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:

It's not wholely obvious/clear to me how this solution would 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;

There's a bug that manifests in limited debug info too - so any
solution that does something like this ^^ seems like it's not
addressing the bug I'm thinking of...

> +
> +  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,

Here's what I had in mind:

diff --git lib/CodeGen/CGDebugInfo.cpp lib/CodeGen/CGDebugInfo.cpp
index 8240c08..8a1b1c8 100644
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -1938,8 +1938,15 @@ llvm::DIType
CGDebugInfo::getOrCreateType(QualType Ty, llvm::DIFile Unit,

   llvm::DIType T = getCompletedTypeOrNull(Ty);

-  if (T.Verify())
+  if (T.Verify()) {
+    // If we're looking for a definition, make sure we have definitions of any
+    // underlying types.
+    if (const TypedefType* TTy = dyn_cast<TypedefType>(Ty))
+      getOrCreateType(TTy->getDecl()->getUnderlyingType(), Unit, Declaration);
+    if (Ty.hasLocalQualifiers())
+      getOrCreateType(QualType(Ty.getTypePtr(), 0), Unit, Declaration);
     return T;
+  }

   // Otherwise create the type.
   llvm::DIType Res = CreateTypeNode(Ty, Unit, Declaration);