[PATCH] D81844: [DebugInfo] Support parsing and dumping of DWARF64 macro units.

Tue Jun 16 04:24:15 PDT 2020

ikudrin marked 3 inline comments as done.
ikudrin added inline comments.

================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:31-32
+  OS << format("macro header: version = 0x%04" PRIx16, Version)
+     << format(", flags = 0x%02" PRIx8, Flags)
+     << ", format = " << FormatString(getDwarfFormat());
   if (Flags & MACRO_DEBUG_LINE_OFFSET)
----------------
dblaikie wrote:
> This is a bit redundant and possibly confusing - the format is given by the flags. Perhaps the flags should be decomposed into their semantic values and rendered that way (& render the raw flag value if it contains other bits set that aren't described by the spec)?
> 
> Though I guess it's not entirely inconsistent with the way we print the format and the length in other sections, when the format is sort of described by the length..
Sorry, but I do not fully understand what you are suggesting to change exactly.

I guess that reporting `Flags` in some cases while omitting it in others might not be a good practice. We discussed something similar for a patch that added reporting the format and the common consensus was that it is better to print the format for both variants rather than only for `DWARF64`.

We could decode the value of `Flags` in each case, but that is already done even if in a bit indirect way. We dump `debug_line_offset` only if the corresponding flag is set and, with this patch, we explicitly dump the format. The only remaining flag, `opcode_operands_table`, is not supported yet. But even if we decide to explicitly explain `Flags`, that should be a separate patch.

================
Comment at: llvm/test/DebugInfo/X86/debug-macro-dwarf64.s:54
+    .asciz "BAZ"
\ No newline at end of file

----------------
jhenderson wrote:
> Nit: no new line at EOF.
Thanks! Sorry. I guess it is time to tackle the settings of my VS Code.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81844