[PATCH] D27634: Make a DWARFDIE class that can help avoid using the wrong DWARFUnit when extracting attributes

[David Blaikie via llvm-commits]

On Fri, Dec 9, 2016 at 3:26 PM Greg Clayton via Phabricator <
reviews at reviews.llvm.org> wrote:

> clayborg added a comment.
>
> Commented a bit with fixes coming soon.
>
>
>
> ================
> Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugInfoEntry.h:29
>
> -/// DWARFDebugInfoEntryMinimal - A DIE with only the minimum required
> data.
> -class DWARFDebugInfoEntryMinimal {
> +/// DWARFDebugInfoEntry - A DIE with only the minimum required data.
> +class DWARFDebugInfoEntry {
> ----------------
> aprantl wrote:
> > Is this comment still accurate?
> Yes it is still accurate.
>
>
> ================
> Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:61
> +      return Die->getOffset();
> +    return -1U;
> +  }
> ----------------
> aprantl wrote:
> > Optional<uint32_t> ?
> > Optional<DwarfOffset>, to allow 64-bit?
> These are wrapping the APIs that used to be in DWARFDebugInfoEntryMinimal
> for now. I will add Optional support to all values in a follow up patch.
>
>
> ================
> Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:71
> +
> +  bool hasChildren() const {
> +    if (Die)
> ----------------
> Many APIs can give you back an DWARFDie object that don't contain a
> DWARFUnit or DWARFDie so it does make sense to answer as correctly as
> possible
>

Do callers really want these "as correct as possible but not actually
meaningful/correct" (because there's no node here) answers? Or should they
be expected to check that the DWARFDie is non-null first before interacting
with it? (like a std::unique_ptr or Optional return value - you must check
it before accessing the underlying object)


>
>
> ================
> Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:77
> +
> +  bool isNULL() const {
> +    return getAbbreviationDeclarationPtr() == nullptr;
> ----------------
> aprantl wrote:
> > operator bool() const?
> > Oh, or does this mean the NULL DIE that is at the end of each group?
> This means valid but NULL Die. I should actually change this do be:
>
> ```
> return Die != nullptr && getAbbreviationDeclarationPtr() == nullptr;
> ```
>
> Because isNULL should only be true if we have a valid DWARFDebugInfoEntry,
> yet it has an abbrev code of zero
>
>
> ================
> Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:91
> +  /// invalid DWARFDie instance if it doesn't.
> +  DWARFDie getSibling() const {
> +    if (isValid())
> ----------------
> aprantl wrote:
> > Perhaps Optional instead of default-construction?
> Good idea.I'll do that in a follow up change once this is in.
>
>
> ================
> Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:124-125
> +  /// \returns true if the attribute was extracted from this DIE.
> +  bool getAttributeValue(dwarf::Attribute Attr,
> +                         DWARFFormValue &FormValue) const;
> +
> ----------------
> Let me do this in a follow up change otherwise this will introduce a ton
> of churn in this change. Is that ok?
>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:159-160
>        savedAddressByteSize = CU->getAddressByteSize();
> -      const auto *CUDIE = CU->getUnitDIE();
> -      if (CUDIE == nullptr)
> +      auto CUDIE = CU->getUnitDIE();
> +      if (!CUDIE)
>          continue;
> ----------------
> It isn't a pointer anymore, it is an object. So this does go along with
> this change. It is invoking the operator bool on DWARFDie.
>
>
> https://reviews.llvm.org/D27634
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161209/da31bd55/attachment.html>