[PATCH] D14294: Add macro support to llvm-dwarfdump tool

[Alexey Samsonov via llvm-commits]

samsonov added inline comments.

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:11
@@ +10,3 @@
+#ifndef LLVM_DEBUGINFO_DWARF_DWARFDEBUGMACRO_H
+#define LLVM_DEBUGINFO_DWARF_DWARFDEBUGMACRO_H
+
----------------
Right, thanks. The header guards in another files were probably not fixed properly when the files were moved.

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:31
@@ +30,3 @@
+    /// The string (name, value) of the macro entry.
+    StringRef MacroStr;
+    // An unsigned integer indicating the identity of the source file.
----------------
Let's use `const char *` here instead.

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:34
@@ +33,3 @@
+    uint16_t File;
+  };
+
----------------
It's unlikely that there will be more than 65536 files, but let's not introduce this restriction here: if we use union we can as well use uint64_t to keep the same size of `Entry`.

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:31
@@ +30,3 @@
+
+    WithColor(OS, syntax::Macro).get() << MacinfoString(E.Type);
+    switch (E.Type) {
----------------
Will it work correctly with `nullptr` string if E.Type is unknown? We might just output `DW_MACINFO_unknown (0x...)` instead of dying here.

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:59
@@ +58,3 @@
+    if (data.getData().data()[Offset] == '\0') {
+      // Reached end of ".debug_macinfo" section.
+      // Skip the '\0' byte and stop parsing.
----------------
Ah, sorry for confusion. ULEB128 encoding of zero is a single zero byte, so you can call
  E.Type = data.getULEB128(&Offset);
and break the look if it's zero.

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:68
@@ +67,3 @@
+    E.Type = data.getULEB128(&Offset);
+
+    switch (E.Type) {
----------------
Thanks! I agree, that this practice is not followed everywhere in DWARF parser at the moment, and hopefully we will fix it one day. It would be nice to have at least the new code follow this.

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:72
@@ +71,3 @@
+      llvm::errs() << "error: Invalid macinfo type\n";
+      return;
+    case DW_MACINFO_define:
----------------
I'd vote for eventually removing all `llvm_unreachable` from parsers... I agree that proper error reporting is out of the scope of this patch, but let's at least not print to stderr. You can just skip parsing the rest of the section, or clear all the entries parsed so far - I'm fine with either of this, as long as you document this in a method declaration in header.

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:89
@@ +88,3 @@
+    case DW_MACINFO_vendor_ext:
+      // DW_MACINFO_vendor_ext is not supported.
+      // Just skip it (read and ignore).
----------------
Hm, if .debug_macinfo in my file contained `DW_MACINFO_vendor_ext`, I would expect to see it in `llvm-dwarfdump` output.

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:96
@@ +95,3 @@
+
+    Macros.push_back(E);
+  }
----------------
See above.


Repository:
  rL LLVM

http://reviews.llvm.org/D14294