[PATCH] D73086: [WIP][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:47
+
+    // FIXME: add support for 64-bit DWARF;
+    uint8_t Flags;
----------------
ikudrin wrote:
> This `FIXME` makes no sense because there is nothing to fix here. Please, remove it.
This is not done, the `FIXME` is still here.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:86
+void DWARFDebugMacro::parse(DWARFContext &Context, DWARFDataExtractor data,
+                            StringRef SectionName) {
   uint64_t Offset = 0;
----------------
As @dblaikie said in the comment for the `dump()` method, "passing explicitly written strings on one side and comparing them to explicitly written strings on the other side is error-prone". You should consider something more reliable, for example, passing a bool argument `IsMacro`.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:96
+        auto Err = M->Header.parseMacroHeader(data, &Offset);
+        auto handleParsingErr = Context.getRecoverableErrorHandler();
+        if (Err)
----------------
 # Do not take the error handler from `Context`. If the method needs to report recoverable errors, it should declare the handler as one of its arguments. That makes the method more flexible, more clear to use, removes unnecessary dependencies and so on.

 # The code does not properly use this handler. It should be used for conditions when the method can continue parsing. That is why it is called "recoverable".

Thus, in this case, as you discontinue the parsing, you should just return `Error` from the method and handle it in the caller.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:141
+      strOffset = data.getRelocatedValue(4 /*Offset Size*/, &Offset);
+      E.MacroStr = Context.getStringExtractor().getCStr(&strOffset);
+      break;
----------------
Consider passing the string extractor as an argument. That will remove the dependency on `Context`.


================
Comment at: llvm/test/DebugInfo/X86/dwarfdump-macro-macinfo.test:1
+RUN: llvm-dwarfdump -v %p/../Inputs/dwarfdump-macro-macinfo.elf-x86_64 | FileCheck %s
+
----------------
It is better to avoid using binary files. They are really hard to maintain. It is also much harder to check if the binary files themselves are correct.


================
Comment at: llvm/test/DebugInfo/X86/unsupported-opcode_operands_table-debug-macro-v5.s:9
+	.short	5                      # Macro information version
+	.byte	4                       # Flags: 32 bit, lineptr present, opcode_operands_table present
+	.long	.Lline_table_start0     # lineptr
----------------
The comment is misleading. `4` means that `debug_line_offset` is absent while `opcode_operands_table` is present.

BTW, where does `lineptr` come from? Why not using the term from the DWARF standard?


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

https://reviews.llvm.org/D73086