[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 7 inline comments as done.
SouraVX added a comment.

In D73086#1866759 <https://reviews.llvm.org/D73086#1866759>, @ikudrin wrote:

>


Hi! Thank You so much for you comments.

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

Indeed it's optional, but I think it should be absolutely present, Please consider @probinson comment in D72828 <https://reviews.llvm.org/D72828>.

> 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.

Not sure, what you meant here, I've tested this implementation with multiple CU's having debug_macro sections. This implementation can dump the entire section[individual and final merged sections in a.out]. It reads/dump header then section content. After reaching to end check's for validity of offset, if valid repeat this cycle until the Offset is invalid.

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

On my TODO list once, debug_macro emission/dumping get's in.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:327
+  while (MacroData.isValidOffset(Offset)) {
+    Macro.parseMacroHeader(MacroData, &Offset);
+    Macro.dumpMacroHeader(OS);
----------------
ikudrin wrote:
> 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.
Yes, I had that in mind while writing this. That can be done, but we'll end with another function *parsev5/dumpv5* or so Do we need to ?
Since most part of dumping/parsing can be reused. 
Or extending the exisiting parse/dump to accomodate v5 header also ?
Or should we opt for fresh start for v5 macro. ?
So I ended writing a separate function for the header. Consider location list for instance, slightly same sort of thing happens their.
Parse/dump Header -> then section contents.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:828
 
 const DWARFDebugMacro *DWARFContext::getDebugMacroDWO() {
   if (MacroDWO)
----------------
ikudrin wrote:
> 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()`.
Yes, that's because of naming convention previously followed, for instance. Line:455 -- 
```
  if (shouldDump(Explicit, ".debug_macinfo", DIDT_ID_DebugMacro,
                  DObj->getMacinfoSection())) {
     getDebugMacro()->dump(OS);
   }
```
I plan to clean this up. After this.


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

https://reviews.llvm.org/D73086