<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Sep 10, 2014 at 10:01 AM, Frédéric Riss <span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@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 style="word-wrap:break-word"><br><div><span class=""><blockquote type="cite"><div>On 10 Sep 2014, at 18:30, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><br><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"><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 9:20 AM, Frédéric Riss<span> </span><span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>></span><span> </span>wrote:<br><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"><br><div><span><blockquote type="cite"><div>On 10 Sep 2014, at 18:10, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><br><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"><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> </span><span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>></span><span> </span>wrote:<br><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><br>> On 09 Sep 2014, at 23:40, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>><br>><br>><br>> On Tue, Sep 9, 2014 at 5:53 AM, Frederic Riss <<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>> wrote:<br>> Hi dblaikie, samsonov, echristo, aprantl,<br>><br>> There is no simple way to get the origin section of a DWARFUnit. It is necessary<br>> to know from which section a Unit has been extracted to be able to resolve<br>> cross-unit references (to find the right list to search fo the reference<br>> target).<br>><br>> 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><br></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><br></div><div>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><br></div></span><div>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><br></div><div><br>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><br>You could do this without faux-RTTI as follows:<br><br>* Add virtual DWARFUnit *DWARFUnit::getSiblingUnitForOffset(uint32_t Offset) = 0<br>* 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><br>Does that make sense? (at least is my explanation understandable - not necessarily a good idea, but making sure I've communicated it clearly)<br><br>Good/bad/indifferent? </div></div></div></blockquote><div><br></div></span><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></div><div>* Add<font face="Menlo-Regular"> 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></div></blockquote><div><br></div><div>Name is just a strawman. Just wanted something to indicate that this finds units in the same "bunch" of units.<br><br>Hmm, thinking about this some more - why is it valid to only look for cross-unit references in the same section as the source unit? Aren't these relocated values? Couldn't they point into other sections? (the most obvious example would be to avoid type unit references (DW_TAG_structure_type + DW_AT_signature) and just directly refer to the type in a type unit)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><font face="Menlo-Regular">* 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></div></blockquote><div><br>I'd probably still favor the CRTP to avoid the need to have the repeated get*UnitForOffset/*UnitLookup functions, but there's not a /lot/ of difference in it.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><font face="Menlo-Regular"><br></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></div></blockquote><div><br></div><div>I believe it is - but I'm in a conference all this week, so a lot of more detailed code review is being shelved until next week - just doing what seems urgent or easy.<br><br>My brief thought on the comparator stuff is that it still seems a bit off, but I haven't got all the state in my head right now.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><br></div><div>Thanks,</div><div>Fred</div><div><div class="h5"><div><br></div><blockquote type="cite"><div><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"><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"><div><div>I have no other current use for the UnitKind than the one bellow.</div><div><br></div><div>Fred</div><div><div><br><blockquote type="cite"><div><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> </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><br>DWARFUnit *DWARFContext::getUnitForOffset(const DWARFUnit *Unit, uint32_t Offset) {<br>   switch (Unit->getKind()) {<br>   case DWARFUnit::UK_Compile:<br>     return getCompileUnitForOffset(Offset);<br>   case DWARFUnit::UK_DWOCompile:<br>     return getDWOCompileUnitForOffset(Offset);<br>   case DWARFUnit::UK_Type:<br>     return getTypeUnitForOffset(Offset);<br>   case DWARFUnit::UK_DWOType:<br>     return getDWOTypeUnitForOffset(Offset);<br>   }<br>   llvm_unreachable("Unkown unit type in getUnitForOffset.");<br> }<br><br>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><br>Fred<br><div><div><br>><br>> This patch adds a field and an accessor to the DWARFUnit that indicates where<br>> it originated from. This information will be used in a subsequent patch to<br>> implement a generic version of DWARFContext::getCompilationUnitForOffset()<br>> that will work with arbitrary unit types.<br>><br>> Note that this is nearly implemented like the start of LLVM-style RTTI for<br>> the DWARFUnit, but it seems that we don't need separate classes to represent<br>> the DWO variants of Units as only their origin changes. Thus we only<br>> introduce this discriminant field.<br>><br>><span> </span><a href="http://reviews.llvm.org/D5264" target="_blank">http://reviews.llvm.org/D5264</a><br>><br>> Files:<br>>   lib/DebugInfo/DWARFCompileUnit.h<br>>   lib/DebugInfo/DWARFContext.cpp<br>>   lib/DebugInfo/DWARFTypeUnit.h<br>>   lib/DebugInfo/DWARFUnit.cpp<br>>   lib/DebugInfo/DWARFUnit.h<br>></div></div></blockquote></div></div></blockquote></div></div></div></div></blockquote></div></div></blockquote></div></div></div><br></div></blockquote></div><br></div></div>