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

Adrian Prantl aprantl at apple.com
Sun Mar 16 12:50:04 PDT 2014


On Mar 16, 2014, at 11:35 AM, Manman Ren <manman.ren at gmail.com> wrote:

> 
> 
> 
> 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.
> 
> We have a few places that we currently do not use DIRef: the list of retained types, scope for DIVariable, and types used in a DIArray ( I need to double check on that, we may have fixed it).
> 
> I am going to add a FIXME to DIVariable.
> 
> For the list of retained types, we have to use MDNodes instead of DIRefs, because we derive the map from type identifiers to MDNodes using the retained types.

Another reason why we need to store the MDNodes in the retained types is that otherwise those types would be eliminated because no other MDNode is referencing them.

> The correct way, I think, is to check if we have already visited a type identifier from the retained types to avoid visiting the same type identifier again.

You mean, perform the lookup only when DwarfDebug is iterating over the retained types to emit them (instead of doing it for every type, as in r203983)?

thanks!
adrian



More information about the llvm-commits mailing list