[PATCH] D28456: DebugInfo: support for DW_FORM_implicit_const

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h:34-38
     /// contained in or the value size varies and must be decoded from the
     /// debug information in order to determine its size.
     Optional<uint8_t> ByteSize;
+    /// Optional value (for DW_FORM_implicit_const).
+    Optional<int64_t> Value;
----------------
These are mutually exclusive, right? (if there's a value, the byte size is zero) - could we coalesce them in some way?


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:85-87
+      if (AP->getDwarfVersion() < 5)
+        report_fatal_error(
+            "DW_FORM_implicit_const is supported starting from DWARFv5");
----------------
assert?


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:413-415
+  case dwarf::DW_FORM_implicit_const:
+    // We do not need to emit anything here: value goes to .debug_abbrev
+    return;
----------------
Move this up to the form present case at the top?

  case dwarf::DW_FORM_implicit_const:
  case dwarf::DW_FORM_flag_present:
    ...


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:467-470
   case dwarf::DW_FORM_sdata:
+    LLVM_FALLTHROUGH;
+  case dwarf::DW_FORM_implicit_const:
     return getSLEB128Size(Integer);
----------------
This seems strange/incorrect - why does implicit_const have a size? Shouldn't it be zero like flag_present (& could roll this case into the flag-present case as above)


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:156
     case DW_FORM_implicit_const:
-      // The implicit value is stored in the abbreviation as a ULEB128, any
+      // The implicit value is stored in the abbreviation as a SLEB128, any
       // there no data in debug info.
----------------
typo 'any' where 'and' is intended, perhaps?


https://reviews.llvm.org/D28456