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

Tue Jun 16 16:00:54 PDT 2020

dblaikie accepted this revision.
dblaikie 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)
----------------
ikudrin wrote:
> 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.
Fair enough - thanks for the explanation.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81844