[llvm] r204107 - DwarfDebug: Only unique retained types instead of all types.

[Eric Christopher]

On Mon, Mar 17, 2014 at 7:53 PM, David Blaikie <dblaikie at gmail.com> wrote:
> On Mon, Mar 17, 2014 at 7:35 PM, Adrian Prantl <aprantl at apple.com> wrote:
>> Author: adrian
>> Date: Mon Mar 17 21:35:03 2014
>> New Revision: 204107
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=204107&view=rev
>> Log:
>> DwarfDebug: Only unique retained types instead of all types.
>> This is a follow-up to r203983 based on feedback from dblaikie and mren (Thanks!)
>> No functionality change.
>
> Thanks!
>
>>
>> Modified:
>>     llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>     llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
>>
>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=204107&r1=204106&r2=204107&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Mon Mar 17 21:35:03 2014
>> @@ -846,8 +846,13 @@ void DwarfDebug::beginModule() {
>>      for (unsigned i = 0, e = EnumTypes.getNumElements(); i != e; ++i)
>>        CU->getOrCreateTypeDIE(EnumTypes.getElement(i));
>>      DIArray RetainedTypes = CUNode.getRetainedTypes();
>> -    for (unsigned i = 0, e = RetainedTypes.getNumElements(); i != e; ++i)
>> -      CU->getOrCreateTypeDIE(RetainedTypes.getElement(i));
>> +    for (unsigned i = 0, e = RetainedTypes.getNumElements(); i != e; ++i) {
>> +      DIType Ty(RetainedTypes.getElement(i));
>> +      // The retained types array by design contains pointers to
>> +      // MDNodes rather than DIRefs. Unique them here.
>> +      DIType UniqueTy(resolve(Ty.getRef()));
>> +      CU->getOrCreateTypeDIE(UniqueTy);
>
> One thought I've had here: should we, rather than resolving the
> references and emitting them, avoid emitting the type if the resolved
> version isn't from this compilation unit?
>
> I'm not sure there's a good reason here, but I'm just tossing it
> around in my head - I wonder if we could get into confusing situations
> (with types as you've described in other recent threads - where two
> ODR-correct definitions, written in different files, may end up
> emitted under the "wrong" compilation unit). I'm not sure if this
> would actually be bad - but, for example, with my line table work it
> might be a tiny bit more verbose to emit a type from the wrong
> compilation unit since it wouldn't take advantage of the comp_dir as
> much (if the type was actually in a header in that directory) or might
> introduce an extra file name in the line table that would  otherwise
> have been avoided.
>
> This is ultra low priority - just sanity checking to see if maybe
> there are issues with emitting the type from the wrong unit.
>

I think you're correct here. The way the code is written we're adding
retained types as we go over the compile unit. We should probably do a
check if the current compile unit is the same as the compile unit for
the type that we're about to emit since the type merging would have
put it in doubt which compile unit a retained type actually belonged
to.

>> +    }
>>      // Emit imported_modules last so that the relevant context is already
>>      // available.
>>      for (unsigned i = 0, e = ImportedEntities.getNumElements(); i != e; ++i)
>>
>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp?rev=204107&r1=204106&r2=204107&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp Mon Mar 17 21:35:03 2014
>> @@ -971,6 +971,8 @@ DIE *DwarfUnit::getOrCreateTypeDIE(const
>>
>>    DIType Ty(TyNode);
>>    assert(Ty.isType());
>> +  assert(*&Ty == resolve(Ty.getRef()) &&
>
> Why's the "*&Ty" required? That seems a bit strange/subtle...
>

Agreed. Adrian?

-eric