<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Sun, Mar 16, 2014 at 12:50 PM, Adrian Prantl <span dir="ltr"><<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
On Mar 16, 2014, at 11:35 AM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>
<br>
><br>
><br>
><br>
> On Sat, Mar 15, 2014 at 8:16 AM, Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br>
><br>
> On Mar 14, 2014, at 6:43 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
> > On Fri, Mar 14, 2014 at 6:37 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br>
> >><br>
> >> On Mar 14, 2014, at 17:59, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> >><br>
> >>> On Fri, Mar 14, 2014 at 4:08 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br>
> >>>> Author: adrian<br>
> >>>> Date: Fri Mar 14 18:08:29 2014<br>
> >>>> New Revision: 203983<br>
> >>>><br>
> >>>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=203983&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=203983&view=rev</a><br>
> >>>> Log:<br>
> >>>> Debug info: Unique types before emitting them to DWARF, where applicable.<br>
> >>>><br>
> >>>> Modified:<br>
> >>>> llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp<br>
> >>>> llvm/trunk/test/Linker/type-unique-odr-a.ll<br>
> >>>><br>
> >>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp<br>
> >>>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp?rev=203983&r1=203982&r2=203983&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp?rev=203983&r1=203982&r2=203983&view=diff</a><br>
> >>>> ==============================================================================<br>
> >>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp (original)<br>
> >>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp Fri Mar 14 18:08:29 2014<br>
> >>>> @@ -958,6 +958,9 @@ DIE *DwarfUnit::getOrCreateTypeDIE(const<br>
> >>>> DIE *ContextDIE = getOrCreateContextDIE(Context);<br>
> >>>> assert(ContextDIE);<br>
> >>>><br>
> >>>> + // Unique the type. This is a noop if the type has no unique identifier.<br>
> >>>> + Ty = DIType(resolve(Ty.getRef()));<br>
> >>><br>
> >>> While this might be a good catch-all - I'm confused/concerned about<br>
> >>> what this implies. How did we get here? We must have some code<br>
> >>> referencing this type by node rather than name, yes? We should<br>
> >>> probably find what that is and fix it because that would mean the type<br>
> >>> won't be metadata node uniqued properly...<br>
> >>><br>
> >>> Might be worth just turning this into an assert instead so we can find<br>
> >>> bugs like this more quickly if they arise.<br>
> >><br>
> >> [+Manman]<br>
> >><br>
> >> 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).<br>
> ><br>
> > That's generally the idea - I don't think we thought that actually ODR<br>
> > uniquing would be needed and just deduplicating by structural<br>
> > equivalence (once the cycles were broken by using the mangled names<br>
> > instead of MDNode references) would be sufficient to minimize the<br>
> > debug info size to something acceptable.<br>
> ><br>
> >> What would be a more natural place to put this? AsmParser?<br>
> ><br>
> > Can we not just fix the DIScopes to use refs instead of MDNode*? I'm<br>
> > not sure what you're thinking we would put in AsmPrinter.<br>
><br>
> 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.<br>
><br>
> 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).<br>
><br>
> I am going to add a FIXME to DIVariable.<br>
><br>
> 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.<br>
<br>
</div></div>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.<br>
<div class=""><br>
> 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.<br>
<br>
</div>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)?<br></blockquote><div><br></div><div>Yes, when visiting the retained list of each CU, we don't want to generate DIEs for DITypes with the same type identifier.<br>
<br></div><div>Cheers,<br>Manman<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
thanks!<br>
<span class="HOEnZb"><font color="#888888">adrian</font></span></blockquote></div><br></div></div>