[PATCH] D78736: [DWARF5]: Added support for dumping strx forms in llvm-dwarfdump

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added a comment.

Please, do not split definitions for sections in the test. It adds no value, just makes it harder to read.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:107-109
+  Error parse(Optional<DWARFUnitVector::iterator_range> Units,
+              Optional<DataExtractor> StringExtractor, DWARFDataExtractor Data,
               bool IsMacro);
----------------
It starts looking that it is better to have two distinct methods in the public interface, `parseMacinfo()` and `parseMacro()`, than adding lots of the optional parameters. They can still share the implementation, though.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:103
+        if (auto MacroOffset = toSectionOffset(CUDIE.find(DW_AT_macros)))
+          MacroToUnits.insert({*MacroOffset, CUDIE});
+  }
----------------
I would probably use `try_emplace()` instead of `insert()` here. It is the same but does not require curly brackets.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:116
     }
+    auto GetContributionUnit = [&]() {
+      const auto DWARFDie = MacroToUnits.find(M->Offset);
----------------
The lambda is simple and is used only in one place. Just move the code there.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:118
+      const auto DWARFDie = MacroToUnits.find(M->Offset);
+      return DWARFDie->second.getDwarfUnit();
+    };
----------------
  - What if there is no record for that offset in the map?

  - Why not just store a pointer to `DWARFUnit` in the map?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:158
       StrOffset = Data.getRelocatedValue(/*OffsetSize=*/4, &Offset);
-      E.MacroStr = StringExtractor.getCStr(&StrOffset);
+      E.MacroStr = StringExtractor.getValue().getCStr(&StrOffset);
+      break;
----------------
`StringExtractor->getCStr()`. But, please, check that `StringExtractor` has a value.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-multi-cu-strx.s:67-69
+       .long   12
+       .short  5
+       .short  0
----------------
Add comments for these values.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-multi-cu-strx.s:83
+       .section        .debug_line,"", at progbits
+       .section        .debug_abbrev,"", at progbits
+       .byte   1                       # Abbreviation Code
----------------
This second part of the `.debug_abbrev` section is not used.


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

https://reviews.llvm.org/D78736