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

[Greg Clayton via Phabricator via llvm-commits]

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


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