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

[Alexey Samsonov via llvm-commits]

samsonov added a subscriber: samsonov.

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFContext.h:167
@@ +166,3 @@
+  /// Get a pointer to the parsed DebugMacro object.
+  const DWARFDebugMacro *DWARFContext::getDebugMacro();
+
----------------
Remove `DWARFContext::`

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:10
@@ +9,3 @@
+
+#ifndef LLVM_LIB_DEBUGINFO_DWARFDEBUGMACRO_H
+#define LLVM_LIB_DEBUGINFO_DWARFDEBUGMACRO_H
----------------
Please fix the header guard (LLVM_DEBUGINFO_DWARF_DWARFDEBUGMACRO_H)

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:30
@@ +29,3 @@
+
+    /// The string (name, value) of the macro entry.
+    StringRef MacroStr;
----------------
Looks like all entries contain either a string, or an integer (file number). Maybe, we can use a union of them?

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:33
@@ +32,3 @@
+    // An unsigned integer indicating the identity of the source file.
+    uint16_t File;
+  };
----------------
Why will uint16_t be enough?

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:58
@@ +57,3 @@
+
+    if (data.getData().data()[Offset] == '\0') {
+      // Reached end of ".debug_macinfo" section.
----------------
You can just call `data.getU8()` and then switch on that, breaking from the loop if parsed value was 0.

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:67
@@ +66,3 @@
+    // 1. Macinfo type
+    E.Type = (MacinfoRecordType)data.getULEB128(&Offset);
+
----------------
This wouldn't work: if the .debug_macinfo section is malformed, and record type is invalid (or it will be some value from new DWARF standard), this code will have undefined behavior. Generally this is the reason we don't use enums from `Dwarf.h` to hold types in parser library - we can't be sure that the values we parse will correspond to defined enum values.

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:71
@@ +70,3 @@
+    default:
+      llvm::errs() << "unexpected macinfo type";
+    case DW_MACINFO_define:
----------------
see below - probably you just need to return that parsing was unsuccessful. Probably (if you're abandoning parsing), you should clear all entries parsed so far.

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:88
@@ +87,3 @@
+    case DW_MACINFO_vendor_ext:
+      // FIXME: DW_MACINFO_vendor_ext is not supported.
+      break;
----------------
This is wrong: DW_MACINFO_vendor_ext is well described: it's a constant followed by a string - you should consume it, and ignore it.

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:92
@@ +91,3 @@
+
+    Macros.push_back(std::move(E));
+  }
----------------
You don't really need `std::move` here: let the compiler add it if necessary.

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:95
@@ +94,3 @@
+  if (data.isValidOffset(Offset))
+    llvm::errs() << "error: failed to consume entire .debug_macinfo section\n";
+}
----------------
It's not nice when library prints some stuff to stderr(). You can either avoid diagnosing this error, or return some value from `parse()` indicating that parsing failed.

================
Comment at: test/DebugInfo/debugmacinfo.test:28
@@ +27,1 @@
+TEST_LINE: file_names[  3]    0 0x00000000 0x00000000 dwarfdump-macro.h
\ No newline at end of file

----------------
Please fix (here and in another places)


Repository:
  rL LLVM

http://reviews.llvm.org/D14294