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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added a comment.

This revision cannot be compiled on its own. It looks like it depends on D72828 <https://reviews.llvm.org/D72828>, but, as @dblaikie suggested in D72828#1824527 <https://reviews.llvm.org/D72828#1824527>, the support in `llvm-dwarfdump` should be implemented first, so that it can be used later for tests. Please, also add some artificial tests here to check `llvm-dwarfdump` itself.

Please, make the `DebugLineOffset` field optional, as that is required by the DWARF standard.

Please, handle the possible errors in the input data while parsing the tables, like insufficient section size, unexpected field values, etc.

The former implementation of `DWARFDebugMacro` represented the whole section. The new implementation can represent only one macro unit, as it is defined in the DWARF5 standard. Please, fix that so that the meaning of the class is not changed.

It may be worth to support `.debug_macro.dwo`, too.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:27
+  };
+  struct Header {
+    /// Macro information number.
----------------
You probably don't need the header fields in a separate struct. That was used in some places to calculate the total size of the header by `sizeof(Header)`, however, as some fields are optional, like `debug_line_offset` and `opcode_operands_table`, or have different sizes in DWARF32 and DWARF64, that approach is not usable anymore.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:52
+    //   a 64-bit DWARF format macro section
+    uint32_t DebugLineOffset;
+  };
----------------
Please, make this field `uint64_t`.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:65
+  void setFlags(uint8_t Flags) {
+    assert(Flags == 2 &&
+           "Only DWARF32 is supported and debug_line_offset must be present.");
----------------
You call this method from `DWARFDebugMacro::parseMacroHeader()`, which reads the value from an untrusted source. Using `assert()` here means that the program just crashes on an unexpected input in Debug build or ignore that input in Release build. Both are wrong. It should print a diagnostic message instead.

By the way, why do you use an explicit constant here if you have already defined the corresponding named constants?


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:67
+           "Only DWARF32 is supported and debug_line_offset must be present.");
+    HeaderData.Flags |=
+        MACRO_FLAG_OFFSET_SIZE_32 | MACRO_FLAG_DEBUG_LINE_OFFSET;
----------------
Using `|=` here seems incorrect because the usage of this method implies setting the whole set of flags.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:70
+  }
+  void setDebugLineOffset(uint32_t DebugLineOffset) {
+    HeaderData.DebugLineOffset = DebugLineOffset;
----------------
Make this `uint64_t`.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:327
+  while (MacroData.isValidOffset(Offset)) {
+    Macro.parseMacroHeader(MacroData, &Offset);
+    Macro.dumpMacroHeader(OS);
----------------
I am pretty sure that `DWARFContext` should not be aware of the internals of that section. All that should be hidden inside the `DWARFDebugMacro` class. `parse()` should parse the whole thing as well as `dump()` also should print everything. Whether the "whole thing" should be the whole section or just one macro unit might be debatable, though.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:828
 
 const DWARFDebugMacro *DWARFContext::getDebugMacroDWO() {
   if (MacroDWO)
----------------
I find the name of this and the following method a bit misleading now. For my taste, they should be renamed to `getDebugMacinfoDWO()` and `getDebugMacinfo()`.


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

https://reviews.llvm.org/D73086