<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Sep 10, 2014 at 11:21 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><div><div class="h5"><blockquote type="cite"><div>On 10 Sep 2014, at 19:34, 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 10:17 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><div><div><blockquote type="cite"><div>On 10 Sep 2014, at 19:08, 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 10:01 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: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"><span> </span>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><div><br></div></div></div><div>All the references that are described in dwarf (except for DW_FORM_ref_sig8) are section relative offsets. The signature stuff needs another interface anyway and we don’t have the infrastructure to handle that yet. </div><span><br><blockquote type="cite"><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><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></div></blockquote><div><br></div></span><div>OK, I’ll try it. But you agree this also means moving or duplicating the search functions from the context into the DWARFUnit?</div></div></div></blockquote><div><br></div><div>I'm not sure I follow there - the CRTP should mean that the function needs to be written only once (in a class template, and it'll be generated twice - once for each of the two derived classes).</div><div> </div></div></div></blockquote><div><br></div></div></div><div>I’m not sure we’re on the same line either. Let’s put things straight:</div><div><br></div><div>Units can be extracted from 4 sections: .debug_info, .debug_types, .debug_info.dwo and .debug_types.dwo. There are 4 corresponding vectors in DWARFContext.</div><div><br></div><div>Today we have DWARFContext::getUnitForOffset() that searches .debug_info. I wanted to add 3 helpers for the 3 other section (eg. getDWOCompileUnitForOffset) and another generic helper (getUnitForOffset) that would select the right search function from the unit kind.</div><div><br></div><div>You suggested that rather than storing a unit kind, we could store a reference to the corresponding vector. If we do this, it seems logical to put the search code also in the unit. The reference to the list in itself is useless. This means putting the equivalent of getCompileUnitForOffset into DWARFUnit. </div><div>There are quite a few calls to getUnitForOffset that don’t have a unit available, which means that we either:</div><div> - leave getCompileUnitForOffset in the context (slight code duplication)</div></div></div></blockquote><div><br></div><div>Could avoid this duplication by refactoring the getUnitForOffset into a standalone function template and call that from the CRTP base and from the current getCompileUnitForOffset. (even in your proposal, this could be used to simplify the getUnitForOffset function you've proposed, of course - by just having it call a single function template, passing in the right collection)</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> - replace calls to getCompileUnitForOffset by "CUSs.empty() ? nullptr : CUs[0].getCompileUnitForOffset()”</div><div><br></div><div>Am I missing something?</div></div></div></blockquote><div><br></div><div>No no, I think you've summarized it fairly accurately.<br></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>Fred</div><div><div class="h5"><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"><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><span><br><blockquote type="cite"><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><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></blockquote><div><br></div></span><div>OK, no worries, I just wanted to make sure it didn’t slip through the cracks. For what it’s worth, I had coded a small testcase exercising all offsets to see the behaviour on the limits and it seemed right (I’m speaking about the last iteration using upper_bound).</div></div></div></blockquote><div><br>*nod* my objection is more feeling and probably feeling about elegance than one of correctness... probably.<br> </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"><div style="word-wrap:break-word"><div><div> </div></div></div></blockquote><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><br></div><div>Fred</div><div><div><div><br></div><blockquote type="cite"><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><br></div><div>Thanks,</div><div>Fred</div><div><div><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></div></blockquote></div></blockquote></div></div></div></div></blockquote></div></div></blockquote></div></div></div><br></div></blockquote></div><br></div></div>