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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:154
       StrOffset = Data.getRelocatedValue(/*OffsetSize=*/4, &Offset);
-      E.MacroStr = StringExtractor.getCStr(&StrOffset);
+      assert(StringExtractor && "String Extractor not found");
+      E.MacroStr = StringExtractor->getCStr(&StrOffset);
----------------
`assert` should not be used for the cases when malicious user input may trigger it.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:162
+      auto MacroContributionOffset = MacroToUnits.find(M->Offset);
+      assert(MacroContributionOffset != MacroToUnits.end() &&
+             "Macro contribution not found");
----------------
The same. Please review other lines in this block to avoid runtime errors/crashes on unexpected input.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-multi-cu-strx.s:82
+       .section        .debug_line,"", at progbits
+       .section        .debug_info,"", at progbits
+.Lcu_begin1:
----------------
The definitions for both CUs should lay under one `.section` directive. In that case, a reader can easily see that there are two CUs in the test. If there are several `.section` directives for one section, it makes reading the test more difficult.

The same for other sections.


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

https://reviews.llvm.org/D78736