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

[Adrian Prantl via llvm-commits]

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

Great! Splitting the migration to Optional<> out into separate change is a good idea, since it will change all the call sites.

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

Absolutely. All the API changes can/should ideally be separate follow-up commits.

thanks,
adrian

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