<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On 10 Sep 2014, at 18:30, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><br class="Apple-interchange-newline"><br style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_quote" style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Wed, Sep 10, 2014 at 9:20 AM, Frédéric Riss<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:friss@apple.com" target="_blank" class="">friss@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On 10 Sep 2014, at 18:10, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:</div><br class=""><div class=""><br class=""><br style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;" class=""><div class="gmail_quote" style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;">On Wed, Sep 10, 2014 at 12:45 AM, Frédéric Riss<span class=""> </span><span dir="ltr" class=""><<a href="mailto:friss@apple.com" target="_blank" class="">friss@apple.com</a>></span><span class=""> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><span class=""><br class="">> On 09 Sep 2014, at 23:40, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:<br class="">><br class="">><br class="">><br class="">> On Tue, Sep 9, 2014 at 5:53 AM, Frederic Riss <<a href="mailto:friss@apple.com" target="_blank" class="">friss@apple.com</a>> wrote:<br class="">> Hi dblaikie, samsonov, echristo, aprantl,<br class="">><br class="">> There is no simple way to get the origin section of a DWARFUnit. It is necessary<br class="">> to know from which section a Unit has been extracted to be able to resolve<br class="">> cross-unit references (to find the right list to search fo the reference<br class="">> target).<br class="">><br class="">> Would it make more sense to give it the actual section (or some kind of alias/handle to it) rather than a flag? That way it'd work for fission (split-dwarf) as well, and type units that may be in comdat sections?<br class=""><br class=""></span>Although this discriminates on origin section, the way it’s used is just to lookup the right Unit list in the context.</blockquote><div class=""><br class=""></div><div class="">OK - then perhaps it should just be a pointer/reference/whatever to the relevant unit list? Or do you need this for other purposes as well?</div></div></div></blockquote><div class=""><br class=""></div></span><div class="">Why not, but what type would you make that list then as it needs to be the same across all Units? Shall I change the SmallVector<std::unique_ptr<DWARF(Compile|Type)Unit> > to SmallVector<std::unique_ptr<DWARFUnit> > and add casts in all current users? Would that look better?</div></div></div></blockquote><div class=""><br class=""></div><div class=""><br class="">Nope, that wouldn't be much use. So the commonality is the result of "getUnitForOffset" - that's generic (DWARFUnit*), though covariant if we cared.<br class=""><br class="">You could do this without faux-RTTI as follows:<br class=""><br class="">* Add virtual DWARFUnit *DWARFUnit::getSiblingUnitForOffset(uint32_t Offset) = 0<br class="">* add a CRTP between DWARFUnit and the derived classes, that would contain the type-specific SmallVector<std::unique_ptr<Derived>> reference (Or pointer, etc) - derived classes are constructed with a reference to that, pass it to their CRTP base. The CRTP base would implement getSiblingUnitForOffset.<br class=""><br class="">Does that make sense? (at least is my explanation understandable - not necessarily a good idea, but making sure I've communicated it clearly)<br class=""><br class="">Good/bad/indifferent? </div></div></div></blockquote><div><br class=""></div><div>I’d need to code it to be sure, but it looks more complicated than my first approach. Here’s another one that I goes in the direction you describe, but does’t need changing the class hierarchy with CRTP:</div><div><br class=""></div><div>* Add<font face="Menlo-Regular" class=""> DWARFUnit *DWARFUnit::getUnitForOffset(uint32_t Offset) = 0 (getSiblingUnitForOffset name doesn’t sound great, sibling is already used in DWARF for inter-tag links. I won’t argue over the name though.)</font></div><div><font face="Menlo-Regular" class="">* Store a pointer to member function DWARFUnit *(DWARFContext::*UnitLookup)(uint32_t Offset) in the Unit that I initialize with the right Context function (once I introduced all the get*UnitForOffset() variants). And just have DWARFUnit::getUnitForOffset() call getContext().*UnitLookup(Offset).</font></div><div><font face="Menlo-Regular" class=""><br class=""></font></div><div>How about that? (BTW, the helpers I’m talking about are waiting on the OffsetComparator fix. Is this one on your list of things to look at?)</div><div><br class=""></div><div>Thanks,</div><div>Fred</div><div><br class=""></div><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class=""><div class="">I have no other current use for the UnitKind than the one bellow.</div><div class=""><br class=""></div><div class="">Fred</div><div class=""><div class="h5"><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;"><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;">It’s not the actual section that matters (the unit already contains a StringRef pointing to the section data). The next patch introduces:<br class=""><br class="">DWARFUnit *DWARFContext::getUnitForOffset(const DWARFUnit *Unit, uint32_t Offset) {<br class=""> switch (Unit->getKind()) {<br class=""> case DWARFUnit::UK_Compile:<br class=""> return getCompileUnitForOffset(Offset);<br class=""> case DWARFUnit::UK_DWOCompile:<br class=""> return getDWOCompileUnitForOffset(Offset);<br class=""> case DWARFUnit::UK_Type:<br class=""> return getTypeUnitForOffset(Offset);<br class=""> case DWARFUnit::UK_DWOType:<br class=""> return getDWOTypeUnitForOffset(Offset);<br class=""> }<br class=""> llvm_unreachable("Unkown unit type in getUnitForOffset.");<br class=""> }<br class=""><br class="">I’m open to any other way to achieve that. And this handles LTO and fission uses (or I can’t see what issues you’re thinking about).<br class=""><br class="">Fred<br class=""><div class=""><div class=""><br class="">><br class="">> This patch adds a field and an accessor to the DWARFUnit that indicates where<br class="">> it originated from. This information will be used in a subsequent patch to<br class="">> implement a generic version of DWARFContext::getCompilationUnitForOffset()<br class="">> that will work with arbitrary unit types.<br class="">><br class="">> Note that this is nearly implemented like the start of LLVM-style RTTI for<br class="">> the DWARFUnit, but it seems that we don't need separate classes to represent<br class="">> the DWO variants of Units as only their origin changes. Thus we only<br class="">> introduce this discriminant field.<br class="">><br class="">><span class=""> </span><a href="http://reviews.llvm.org/D5264" target="_blank" class="">http://reviews.llvm.org/D5264</a><br class="">><br class="">> Files:<br class="">> lib/DebugInfo/DWARFCompileUnit.h<br class="">> lib/DebugInfo/DWARFContext.cpp<br class="">> lib/DebugInfo/DWARFTypeUnit.h<br class="">> lib/DebugInfo/DWARFUnit.cpp<br class="">> lib/DebugInfo/DWARFUnit.h<br class="">></div></div></blockquote></div></div></blockquote></div></div></div></div></blockquote></div></div></blockquote></div><br class=""></body></html>