[llvm] r203983 - Debug info: Unique types before emitting them to DWARF, where applicable.

[David Blaikie]

On Fri, Mar 14, 2014 at 6:37 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
> On Mar 14, 2014, at 17:59, David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Fri, Mar 14, 2014 at 4:08 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>> Author: adrian
>>> Date: Fri Mar 14 18:08:29 2014
>>> New Revision: 203983
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=203983&view=rev
>>> Log:
>>> Debug info: Unique types before emitting them to DWARF, where applicable.
>>>
>>> Modified:
>>>    llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
>>>    llvm/trunk/test/Linker/type-unique-odr-a.ll
>>>
>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp?rev=203983&r1=203982&r2=203983&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp (original)
>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp Fri Mar 14 18:08:29 2014
>>> @@ -958,6 +958,9 @@ DIE *DwarfUnit::getOrCreateTypeDIE(const
>>>   DIE *ContextDIE = getOrCreateContextDIE(Context);
>>>   assert(ContextDIE);
>>>
>>> +  // Unique the type. This is a noop if the type has no unique identifier.
>>> +  Ty = DIType(resolve(Ty.getRef()));
>>
>> While this might be a good catch-all - I'm confused/concerned about
>> what this implies. How did we get here? We must have some code
>> referencing this type by node rather than name, yes? We should
>> probably find what that is and fix it because that would mean the type
>> won't be metadata node uniqued properly...
>>
>> Might be worth just turning this into an assert instead so we can find
>> bugs like this more quickly if they arise.
>
> [+Manman]
>
> Interesting. It appears that the only callers of DIScope::getRef() are in DIBuilder, which explains why before this change, no ODR/name-based uniquing happened during LTO. (Only fully identical MDNodes would be uniqued).

That's generally the idea - I don't think we thought that actually ODR
uniquing would be needed and just deduplicating by structural
equivalence (once the cycles were broken by using the mangled names
instead of MDNode references) would be sufficient to minimize the
debug info size to something acceptable.

> What would be a more natural place to put this? AsmParser?

Can we not just fix the DIScopes to use refs instead of MDNode*? I'm
not sure what you're thinking we would put in AsmPrinter.