<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Mon, Jun 19, 2017 at 4:04 PM Paul Robinson via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">probinson added a comment.<br>
<br>
I'm not super excited about the hybrid form, but always unpacking a Unit inside the DWARFFormValue methods felt kind of expensive.<br>
<br>
I have, over the weeks, contemplated inventing a fake kind of Unit for the line table, which would have all the Right Stuff in it to let DWARFFormValue do its thing. This of course is getting away from the hybrid in the opposite way--make callers provide a Unit-like object, instead of making most callers unpack a Unit into some other kind of info/helper class. Building a fake Unit feels a little icky, and I think might not be especially cheap, but as it's only once per line-table it might be okay...  I've been resisting it because a line table is-not-a Unit, what is your opinion?  (Am I making sense?)<br></blockquote><div><br></div><div>What about a common/trivial base type that the unit could derive from (essentially making FormValueHelper a base of DWARF*Unit - again, might be good to find a more general name for it). That way DWARF*Units wouldn't have to be remassaged into the FormValueHelper.<br><br>Alternatively composition - have the DWARF*Unit have a FormValueHelper as a member to store these things, and could return a reference to that for form evaluations.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<br>
================<br>
Comment at: llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp:655-658<br>
+  if (U) {<br>
+    if (const char *Str = U->getStringExtractor().getCStr(&Offset))<br>
+      return Str;<br>
+  } else if (const char *Str = C->getStringExtractor().getCStr(&Offset))<br>
----------------<br>
dblaikie wrote:<br>
> This sort of fallback seems error prone or confusing. Are there cases where the first part is necessary? If so, it seems like there would be places where the second/new part is incorrect?<br>
I had discovered that the first part is necessary for .dwo cases (~10 tests), because of things like .debug_types.dwo wanting to get its strings from .debug_str.dwo not .debug_str, and so forth.  The dumper constructs a Unit that has either the .dwo or non-.dwo section handles in it, so that getStringExtractor will point to the correct section.  I can't get that directly out of the Context unless DWARFFormValue knows a whole lot more about its environment than I would really like.<br></blockquote><div><br>Seems to me like it'd be good to ensure the functionality is general - if accessing the string extractor from the context isn't sufficiently general, perhaps the DWARFFormValue should hold a pointer/reference (or perhaps copy - have to consider the lifetime implications, etc) of the FormValueHelper.<br><br>Because there is a line table in the .dwo file, for type units - so sounds like it'd be necessary to support these forms there as well.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<br>
<a href="https://reviews.llvm.org/D33155" rel="noreferrer" target="_blank">https://reviews.llvm.org/D33155</a><br>
<br>
<br>
<br>
</blockquote></div></div>