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

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX marked 24 inline comments as done.
SouraVX added a comment.

In D73086#1905457 <https://reviews.llvm.org/D73086#1905457>, @probinson wrote:

> I believe my comments have been addressed (with corrections supplied by @ikudrin ,thanks!) but there are others still pending.


Thanks for this!
Since this revision has a long history of changes, sorry if I un-intentionally missed some comments to be addressed. I'm browsing and marking comments done from beginning looking for un-addressed comments, if still I misses something, Please point out.
To summarize following comments are already addressed-
@dblaikie 
-Making implementation independent of producer.
-independent testing with llvm-mc.
-Making implementation of parse per CU contribution in one DWARFDebugMacro.
@ikudrin 
-Making DebugLineOffset conditional
-Moving parseMacroHeader/dumpHeader to be part of MacroHeader.
-Making DebugLineOffset uint64_t



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:108-117
     case DW_MACINFO_define:
     case DW_MACINFO_undef:
       // 2. Source line
-      E.Line = data.getULEB128(&Offset);
+      E.Line = data.getULEB128(Offset);
       // 3. Macro string
-      E.MacroStr = data.getCStr(&Offset);
+      E.MacroStr = data.getCStr(Offset);
+      break;
----------------
dblaikie wrote:
> This switch has a mix of DW_MACRO and DW_MACINFO enumerators in it, which seems suspicous/questionable.
> 
> Perhaps it'd be better to use DW_MACRO macros exclusively and include a comment prior to the switch explaining that the macinfo values are a strict subset (with identical constant values) of the macro values that existed previously?
`DW_MACINFO_vendor_ext` is not part of v5, even if we replace all DW_MACINFO* in this switch. We still have to keep `DW_MACINFO_vendor_ext` for backward compatibility.
After changing, this will look some what close to
```switch...
DW_MACRO_define:
DW_MACRO_undef:
...
DW_MACINFO_vendor_ext:

```
Should we go forward with this @dblaikie 


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

https://reviews.llvm.org/D73086