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

[Amjad Aboud via llvm-commits]

aaboud marked 5 inline comments as done.
aaboud added a comment.

Hi Alexey,
Thanks for your comments.
I addressed most of them, however, there are couple of them that I am not sure how to resolve.

Please, see my answers below.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:10
@@ +9,3 @@
+
+#ifndef LLVM_LIB_DEBUGINFO_DWARFDEBUGMACRO_H
+#define LLVM_LIB_DEBUGINFO_DWARFDEBUGMACRO_H
----------------
samsonov wrote:
> Please fix the header guard (LLVM_DEBUGINFO_DWARF_DWARFDEBUGMACRO_H)
I fixed this in this file.
However, all the headers files in this directory have the wrong guard name.

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:30
@@ +29,3 @@
+
+    /// The string (name, value) of the macro entry.
+    StringRef MacroStr;
----------------
samsonov wrote:
> Looks like all entries contain either a string, or an integer (file number). Maybe, we can use a union of them?
I tried to do that before, but it does not work when one of the fields is a class like StringRef.
This is the error I an getting:

'llvm::DWARFDebugMacro::Entry::MacroStr' : illegal union member; type 'llvm::StringRef' has a user-defined constructor or non-trivial default constructor.

Should I use char* and length instead of StringRef? Or just keep the code like this?

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:33
@@ +32,3 @@
+    // An unsigned integer indicating the identity of the source file.
+    uint16_t File;
+  };
----------------
samsonov wrote:
> Why will uint16_t be enough?
I really do not know.
I just copied that from "DWARFDebugLine.h", it is using uint16_t for File ID.

 An unsigned integer indicating the identity of the source file
    // corresponding to a machine instruction.
    uint16_t File;


Should I use a bigger integer type?

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:58
@@ +57,3 @@
+
+    if (data.getData().data()[Offset] == '\0') {
+      // Reached end of ".debug_macinfo" section.
----------------
samsonov wrote:
> You can just call `data.getU8()` and then switch on that, breaking from the loop if parsed value was 0.
I am not sure about that.
If I call data.getU8(), it will read and remove the first byte.
If that byte is not '\0', I cannot use it to calculate the Type, as the Type is a ULEB128.

Am I missing something?

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:67
@@ +66,3 @@
+    // 1. Macinfo type
+    E.Type = (MacinfoRecordType)data.getULEB128(&Offset);
+
----------------
samsonov wrote:
> 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.
Once again, I was following other section implementation: "DWARFDebugLocDWO::parse()", it is casting to a dwarf enumeration.

Anyway, I just replaces the dwarf macro type with uint32_t.

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:71
@@ +70,3 @@
+    default:
+      llvm::errs() << "unexpected macinfo type";
+    case DW_MACINFO_define:
----------------
samsonov wrote:
> 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.
Yes, I missed the "return;" statement here.
But I do not think we need to clean the parsed entries, let the user get an indication on at what point the object file get corrupted.

Notice that "DWARFDebugFrame.cpp" is using llvm_unreachable for corrupted code!



================
Comment at: lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:92
@@ +91,3 @@
+
+    Macros.push_back(std::move(E));
+  }
----------------
samsonov wrote:
> You don't really need `std::move` here: let the compiler add it if necessary.
I will fix that in this file.
However, I copied it from "DWARFDebugLoc.cpp", maybe it should be fixed there as well.

================
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";
+}
----------------
samsonov wrote:
> 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.
Totally agree.

I will skip this error check for this patch.
Because, as this llvm-dwarfdump tool /LLVMDebugInfoDwarf library does not support error result communication, I do not think that we should do it in this patch.

I see that there is a place to fix this code (the one added in this patch and the existing code for other sections), however, I believe it is out of the scope of this patch.

Do you think otherwise?


Repository:
  rL LLVM

http://reviews.llvm.org/D14294