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

[David Blaikie via llvm-commits]

On Mon, Jun 19, 2017 at 4:04 PM Paul Robinson via Phabricator <
reviews at reviews.llvm.org> wrote:

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

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.

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.


>
>
>
> ================
> 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.
>

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.

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.


>
>
>
> https://reviews.llvm.org/D33155
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170619/451b870c/attachment.html>