[PATCH] D73086: [DWARF5] Added support for debug_macro section parsing and dumping in llvm-dwarfdump.

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:84
+    uint8_t getFlags() const { return Header.Flags; }
+    uint32_t getDebugLineOffset() const { return Header.DebugLineOffset; }
+    void setVersion(uint16_t Version) { Header.Version = Version; }
----------------
This should be `uint64_t`.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:88
+    Expected<bool> setFlags(uint8_t Flags) {
+      if (Flags == MACRO_OFFSET_SIZE) {
+        Error Err =
----------------
So, the code pretends to support DWARF64 if the `debug_line_offset` field is present, right?


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:114
+  /// Parse the debug_macro header.
+  Expected<bool> parseMacroHeader(struct MacroList *Macros,
+                                  DWARFDataExtractor data, uint64_t *Offset);
----------------
This functionality should be inside `MacroHeader`. In this case, there will be no need for setters in `MacroHeader`.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:25
+               Macros.getVersion(), Macros.getFlags());
+  if (Macros.getFlags() == MACRO_DEBUG_LINE_OFFSET) {
+    OS << format(", debug_line_offset = 0x%04" PRIx32 "\n",
----------------
This will print the `debug_line_offset` field only if the format is DWARF32 and no `opcode_operands_table` is present. This is not correct in view of further changes. Please check the flag in a proper way, with bitwise operators.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:25
+               Macros.getVersion(), Macros.getFlags());
+  if (Macros.getFlags() == MACRO_DEBUG_LINE_OFFSET) {
+    OS << format(", debug_line_offset = 0x%04" PRIx32 "\n",
----------------
ikudrin wrote:
> This will print the `debug_line_offset` field only if the format is DWARF32 and no `opcode_operands_table` is present. This is not correct in view of further changes. Please check the flag in a proper way, with bitwise operators.
You do not need brackets if there is only one instruction in the branch.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:26
+  if (Macros.getFlags() == MACRO_DEBUG_LINE_OFFSET) {
+    OS << format(", debug_line_offset = 0x%04" PRIx32 "\n",
+                 Macros.getDebugLineOffset());
----------------
This should be `PRIx64`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73086/new/

https://reviews.llvm.org/D73086