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

[Adrian Prantl]

On Mar 14, 2014, at 6:43 PM, David Blaikie <dblaikie at gmail.com> wrote:

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

Looking at the test case again, it looks like the problem is actually the list of retained types in the module  that doesn’t use DIScopeRefs. I will see if that makes the problem disappear to a point were we can make the above code into an assertion as you suggested.

thanks,
adrian