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

Mon May 15 11:29:32 PDT 2017

On Mon, May 15, 2017 at 11:09 AM Paul Robinson via Phabricator <
reviews at reviews.llvm.org> wrote:

> probinson added inline comments.
>
>
> ================
> Comment at: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:57
>    ValueType Value;              // Contains all data for the form.
> -  const DWARFUnit *U = nullptr; // Remember the DWARFUnit at extract time.
> +  std::shared_ptr<const DWARFUnitProxy> U; // Info useful during
> extraction.
>
> ----------------
> dblaikie wrote:
> > My best guess is that sizeof(DWARFFormValue) isn't so important this
> needs to be indirected. It looks like most/all DWARFFormValues are
> transient, when some code is visiting the current attribute or form - I
> don't see code manifesting in a container "all forms/attributes for a DIE"
> for example (nor all attributes/forms in an entire file, etc).
> >
> > So maybe having this as a value member, rather than a shared_ptr would
> reduce complexity?
> >
> > Also - while I understand it's convenient to wrap DWARFUnit in the proxy
> so existing code doesn't need to be touched, I'm not sure it's that much of
> an improvement in readability - but I could be wrong - compared to having a
> few members and using those directly? (indeed most code still wouldn't
> change because it could use the DWARFUnit *U that was there before/would
> remain - and a few places would use the few extra new state/variables)
> You have touched on a couple of different things here.
>
> Using a value member instead of shared_ptr seems fine, it would make the
> diff a whole lot longer but most of the extra changes would be replacing
> `U->foo` with `U.foo`.
>
> Yes we could build a helper class that has value members with all the
> right info.  But then we are paying some nonzero cost to extract/compute
> those values (e.g., size of a ref-addr) when constructing the class, but
> most of the time we won't actually need all that.

Not sure I understand the cost you're describing - we're talking about the
same thing, right - having the members of the current DWARFUnitProxy as
members of DWARFFormValue instead (rather than indirected through either a
value type or shared_ptr handle)?

Not sure I understand where the cost would come from for that anymore than
for the current situation - they're 4 primitive types, they don't cost
anything to initialize - and the cost would occur even if they were in a
separate struct but a value member (instead of shared_ptr) of
DWARFFormValue.

>   And remember that with all the new indirection in v5, we are talking
> about handles on quite a few different sections that we'd need to copy over
> into the new helper, on the off chance that we'd need them.  Or make the
> constructor a lot more complicated, smart enough to work out which pieces
> it needs based on the Form...
>
> If we really wanted to get radical, DWARFFormValue should be handed the
> DWARFContext rather than the DWARFUnit, along with the few bits of
> unit-specific info (32/64 format, version, address size).  Then we query
> the context as needed, instead of the unit.  That minimizes copying while
> retaining the unit/line-table flexibility.
>
> I'm open to suggestions.
>
>
>
> ================
> Comment at: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:82
>    /// kind of forms that depend on Unit information are disallowed.
> +  /// \param Data The DataExtractor to use.
> +  /// \param OffsetPtr The offset within DataExtractor where the data
> starts.
> ----------------
> dblaikie wrote:
> > Does the LLVM style guide say to document all parameters? (I guess the
> clang doxy warnings flag things like this if they're not document?)
> >
> > It'd be nice if there were a tidier way to just say "there's nothing to
> add here" - because I don't think "The DataExtractor to use" really adds
> anything compared to what the generated documentation or function signature
> already has.
> Seems to me I did get warnings or something when not all parameters were
> documents.
> If you want to propose a "nothing to see here" case in the coding
> standard, knock yourself out.
>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:143-144
>                       std::vector<DWARFDebugLine::FileNameEntry>
> &FileNames) {
> +  dwarf::DwarfFormat Format =
> +      IsDwarf64 ? dwarf::DwarfFormat::DWARF64 :
> dwarf::DwarfFormat::DWARF32;
> +
> ----------------
> dblaikie wrote:
> > I know DWARF64 support is starting to be considered in a few places -
> but is this tested?
> Huh.  No, I missed that, I will add a test.
> IMHO it would not be the end of the world for the dumpers to be able to
> handle DWARF64.  The day will likely come when somebody really can't work
> around the lack.
>

Oh, yeah I'm not opposing DWARF64 support by any means - seems like
perfectly useful stuff to support, just making sure it's tested, etc.

>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:135
> +      return U->getStringOffsetSectionItem(Index, Result);
> +    llvm_unreachable("Invalid use of DWARFUnitProxy");
> +  }
> ----------------
> dblaikie wrote:
> > avoid branch-to-unreachable, instead "assert(U)" at the start of the
> function (similarly for other cases above/in this patch)
> Hmm so a release build would get a seg fault instead of a more intelligent
> error?  I can do what you said but an actual diagnostic instead of a seg
> fault seems pretty cheap.
>

LLVM can/does optimize on the basis of llvm_unreachable - it could optimize
out the boolean condition (knowing it must always be true, because false
leads to UB), so it's no better here, quality/failure-wise.

>
>
> https://reviews.llvm.org/D33155
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170515/3700be75/attachment-0001.html>