[PATCH] D78500: [DWARF5]:Added support for .debug_macro.dwo section in llvm-dwarfdump

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added a comment.

I am supporting @dblaikie in that the patch should be split. The part of adding `*_strx` forms should be separated from adding the support of `debug_macro.dwo`. The parts look independent of each other and can be added in any order.

And please, reduce the tests.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:107-109
+  Error parse(DWARFUnitVector::iterator_range CUs,
+              Optional<DWARFDataExtractor> StringOffsetsExtractor,
+              DataExtractor StringExtractor, DWARFDataExtractor Data,
----------------
`CUs` and `StringExtractor` should be optional because they are not needed when parsing `debug_macinfo[.dwo]`.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:299
   auto ParseAndDump = [&](DWARFDataExtractor &Data, bool IsMacro) {
-    if (Error Err = Macro->parse(getStringExtractor(), Data, IsMacro)) {
+    Optional<DWARFDataExtractor> StrOffsetsExtractor =
+        SectionType != MacroDwoSection ? getStringOffsetsExtractor()
----------------
This should not be filled if `IsMacro == false`. This way you can just pass it later in the code without additional condition.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:303
+    if (Error Err = Macro->parse(
+            compile_units(),
+            // String Offsets extractor is only needed for debug_macro[.dwo]
----------------
This should be `dwo_compile_units()` if you parse a DWO section, no?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:309-310
+                : StrOffsetsExtractor,
+            SectionType != MacroDwoSection ? getStringExtractor()
+                                           : getStringDWOExtractor(),
+            Data, IsMacro)) {
----------------
The strings section is not needed when parsing `.debug_macinfo[.dwo]`, right? The argument should be optional, too. As it is written at the moment, it looks a bit misleading when you pass the extractor for `.debug_str` when parsing `.debug_macinfo.dwo`.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:118
 
+    auto GetStrOffsetsBase = [&]() {
+      for (auto &CU : CUs)
----------------
SouraVX wrote:
> ikudrin wrote:
> > What if there are two or more CUs which use separate contributions to the macro section with different string offsets bases?
> Not sure what you meant here, this patch provide support for this situation too. 
> The test case `debug-macro-multi-cu-strx.s` contains multiple CUs(2 CU) having separated contributions to macro section with different string offsets base.
> 
Ah, I see, `M->Offset` is hidden deep inside the lambda. A bit counterintuitive.

Anyway, this approach is very inefficient. The set of compile units is scanned each time `*_strx` code is encountered. And the implementation is very limited in the sense of what it can handle. `DWARFUnit` already has the code to find the contribution in the string offsets sections, no need to reimplement it again. Please, take a look at dumping the string offsets sections, `dumpDWARFv5StringOffsetsSection()` for now or `dumpStringOffsetsSection()` after D78555, in `DWARFContext.cpp`. I guess the approach may be adopted here.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:163
+      uint64_t IndexOffset = StrOffsetBases + OffsetIndex * /*EntrySize=*/4;
+      uint64_t StrOffset = StringOffsetsExtractor.getU32(&IndexOffset);
+      E.MacroStr = StringExtractor.getCStr(&StrOffset);
----------------
SouraVX wrote:
> dblaikie wrote:
> > Does the StringOffsetsExtractor already know which contribution it's reading from (if you have two objects linked together, with two string offsets contributions, would this code correctly read the desired one?)
> > 
> > Speaking of: these forms (_strx) aren't unique to debug_macro.dwo, they can be used in debug_macro - perhaps it'd be good to separate adding support for these new forms & testing them in debug_macro (non-dwo) support, then, separately, adding support for debug_macro.dwo?
> > Does the StringOffsetsExtractor already know which contribution it's reading from (if you have two objects linked together, with two string offsets contributions, would this code correctly read the desired one?)
> In a `dwo` object there can't be multiple contributions, and perhaps the naming(of the Extractor) is confusing here, I 'll modify that too `getStringOffsetsDWOExtractor` as that's what has been passed by `DWARFContext`.
> 
> >Speaking of: these forms (_strx) aren't unique to debug_macro.dwo, they can be used in debug_macro - perhaps it'd be good to separate adding support for these new forms & testing them in debug_macro (non-dwo) support, then, separately, adding support for debug_macro.dwo?
> Yes they can be used in macro (non-dwo) too, emission from clang/llvm won't be a problem. But parsing/dumping is where it will add complexity that IMO is un-justified.
> Added complexity in a sense, for example to parse a single `strx` entry form we have to look for its CU(from a list of all CU's) contribution to `debug_str_offsets` section i.e `DW_AT_str_offsets_base`, then correctly parsing it to set `StrOffsetsBase` and subsequent actual macro defined in `debug_str` section.
> (For getting CU contribution, we could use `DW_AT_macros` to search among the list of CU's then extract `DW_AT_str_offsets_base` from it ?)  
> 
> This whole setup looks a bit complex/error-prone at-least in contrast to the simplicity provided by `strp` entry form in case multiple CU's linked in a normal object.
> 
> 
> 
> In a dwo object there can't be multiple contributions
What about `dwp`?


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-dwo.s:33
+	.section	.debug_str_offsets.dwo,"e", at progbits
+	.long	28
+	.short	5
----------------
Please, avoid hardcoding the length. Add a pair of labels and use an expression.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-dwo.s:55
+	.long	9
+	.long	31
+	.long	39
----------------
Please, avoid hardcoding the offsets. Use something like `.long .Linfo_string3-.debug_str.dwo` instead.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-dwo.s:68-71
+	.byte	2                       # DW_AT_producer
+	.short	12                     # DW_AT_language
+	.byte	3                       # DW_AT_name
+	.byte	4                       # DW_AT_dwo_name
----------------
These attributes are not required for the test, right?


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

https://reviews.llvm.org/D78500