<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Dec 9, 2016 at 3:26 PM Greg Clayton via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">clayborg added a comment.<br class="gmail_msg">
<br class="gmail_msg">
Commented a bit with fixes coming soon.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugInfoEntry.h:29<br class="gmail_msg">
<br class="gmail_msg">
-/// DWARFDebugInfoEntryMinimal - A DIE with only the minimum required data.<br class="gmail_msg">
-class DWARFDebugInfoEntryMinimal {<br class="gmail_msg">
+/// DWARFDebugInfoEntry - A DIE with only the minimum required data.<br class="gmail_msg">
+class DWARFDebugInfoEntry {<br class="gmail_msg">
----------------<br class="gmail_msg">
aprantl wrote:<br class="gmail_msg">
> Is this comment still accurate?<br class="gmail_msg">
Yes it is still accurate.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:61<br class="gmail_msg">
+      return Die->getOffset();<br class="gmail_msg">
+    return -1U;<br class="gmail_msg">
+  }<br class="gmail_msg">
----------------<br class="gmail_msg">
aprantl wrote:<br class="gmail_msg">
> Optional<uint32_t> ?<br class="gmail_msg">
> Optional<DwarfOffset>, to allow 64-bit?<br class="gmail_msg">
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.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:71<br class="gmail_msg">
+<br class="gmail_msg">
+  bool hasChildren() const {<br class="gmail_msg">
+    if (Die)<br class="gmail_msg">
----------------<br class="gmail_msg">
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<br class="gmail_msg"></blockquote><div><br></div><div>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)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:77<br class="gmail_msg">
+<br class="gmail_msg">
+  bool isNULL() const {<br class="gmail_msg">
+    return getAbbreviationDeclarationPtr() == nullptr;<br class="gmail_msg">
----------------<br class="gmail_msg">
aprantl wrote:<br class="gmail_msg">
> operator bool() const?<br class="gmail_msg">
> Oh, or does this mean the NULL DIE that is at the end of each group?<br class="gmail_msg">
This means valid but NULL Die. I should actually change this do be:<br class="gmail_msg">
<br class="gmail_msg">
```<br class="gmail_msg">
return Die != nullptr && getAbbreviationDeclarationPtr() == nullptr;<br class="gmail_msg">
```<br class="gmail_msg">
<br class="gmail_msg">
Because isNULL should only be true if we have a valid DWARFDebugInfoEntry, yet it has an abbrev code of zero<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:91<br class="gmail_msg">
+  /// invalid DWARFDie instance if it doesn't.<br class="gmail_msg">
+  DWARFDie getSibling() const {<br class="gmail_msg">
+    if (isValid())<br class="gmail_msg">
----------------<br class="gmail_msg">
aprantl wrote:<br class="gmail_msg">
> Perhaps Optional instead of default-construction?<br class="gmail_msg">
Good idea.I'll do that in a follow up change once this is in.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:124-125<br class="gmail_msg">
+  /// \returns true if the attribute was extracted from this DIE.<br class="gmail_msg">
+  bool getAttributeValue(dwarf::Attribute Attr,<br class="gmail_msg">
+                         DWARFFormValue &FormValue) const;<br class="gmail_msg">
+<br class="gmail_msg">
----------------<br class="gmail_msg">
Let me do this in a follow up change otherwise this will introduce a ton of churn in this change. Is that ok?<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:159-160<br class="gmail_msg">
       savedAddressByteSize = CU->getAddressByteSize();<br class="gmail_msg">
-      const auto *CUDIE = CU->getUnitDIE();<br class="gmail_msg">
-      if (CUDIE == nullptr)<br class="gmail_msg">
+      auto CUDIE = CU->getUnitDIE();<br class="gmail_msg">
+      if (!CUDIE)<br class="gmail_msg">
         continue;<br class="gmail_msg">
----------------<br class="gmail_msg">
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.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D27634" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D27634</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>