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

[David Blaikie]

On Sat, Mar 15, 2014 at 8:16 AM, Adrian Prantl <aprantl at apple.com> wrote:
>
> 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! Would it make sense to revert this patch for now until you/we
figure this out? (don't mind either way too much - given that the
change is small - but don't want to lose track of this)