[PATCH] D33155: [DWARFv5] Support FORM_strp in the line table header

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

I'm not super excited about the hybrid form, but always unpacking a Unit inside the DWARFFormValue methods felt kind of expensive.

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?)



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp:655-658
+  if (U) {
+    if (const char *Str = U->getStringExtractor().getCStr(&Offset))
+      return Str;
+  } else if (const char *Str = C->getStringExtractor().getCStr(&Offset))
----------------
dblaikie wrote:
> 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?
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.



https://reviews.llvm.org/D33155