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

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX marked 2 inline comments as done.
SouraVX 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.

Okay, I'll separate them, but first let us reach to a consensus on the approach used here and the way forward.

> And please, reduce the tests.

Briefly stated the reason why they are a bit long, assuming D78555 <https://reviews.llvm.org/D78555> will address this, I'll be happy to reduce them to bare minimal.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:118
 
+    auto GetStrOffsetsBase = [&]() {
+      for (auto &CU : CUs)
----------------
ikudrin wrote:
> 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.
> 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. 

Primary problem with `_strx` form is that they are just indices so you have to map them to there corresponding CU get `str_offset_base` and using that dump. otherwise we'll end up doing this in-correctly.

And for mapping indices correctly to CU, we need to find the CU in list CU's based on macro contributions(since `strx` is contributing here). This is done here using `DW_AT_macros` to map to appropriate CU, subsequently getting the `DW_AT_str_offsets_base` and proceed forward.

> Anyway, this approach is very inefficient.

Yea, that was one of the reason I was inclined to not to use `_strx` in `debug_macro` section at-least(In my first revision). But for `debug_macro.dwo` `_strx` seems a good fit there and `_strp` can't be allowed since that involves relocation .

BTW Is there a better/efficient way to accomplish this that I'm missing out here ?  


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


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-dwo.s:55
+	.long	9
+	.long	31
+	.long	39
----------------
ikudrin wrote:
> Please, avoid hardcoding the offsets. Use something like `.long .Linfo_string3-.debug_str.dwo` instead.
These are clang generated assembly, although slightly tweaked to fit our purpose. I was also surprised to see raw offsets, but as of now this is the way it is. 
Do you want me to replace them with your approach ?


================
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
----------------
ikudrin wrote:
> These attributes are not required for the test, right?
I think(at the moment, writing this patch), they are needed. Since `str_offsets` section dumping relies on the `MaxVersion` for discriminating between v5 and pre-standardized `str_offsets` section. So CU Header is needed, that's how `Maxversion` is set to `5`.

```
if (MaxVersion >= 5)
     dumpDWARFv5StringOffsetsSection(OS, DumpOpts, SectionName, Obj ...

```
But I guess after D78555, this shouldn't be the problem :) Thanks for this!


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

https://reviews.llvm.org/D78500