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

Thu Apr 23 02:41:11 PDT 2020

ikudrin added a comment.

In D78500#1998634 <https://reviews.llvm.org/D78500#1998634>, @SouraVX wrote:

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

I can't imagine how D78555 <https://reviews.llvm.org/D78555> can help with that.

================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:118

+    auto GetStrOffsetsBase = [&]() {
+      for (auto &CU : CUs)
----------------
SouraVX wrote:
> 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 ?  
> BTW Is there a better/efficient way to accomplish this that I'm missing out here ?

As I've said, take a look at `dumpDWARFv5StringOffsetsSection()`.

================
Comment at: llvm/test/DebugInfo/X86/debug-macro-dwo.s:55
+	.long	9
+	.long	31
+	.long	39
----------------
SouraVX wrote:
> 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 ?
The test is for `llvm-dwarfdump`, not `clang`. `clang` can use raw values because it produces the output primarily for other tools, not for people to read it; thus, if it can calculate the exact value, it does not need to use descriptive expressions. On the other hand, the test files, like any kind of source files, should be aimed for people in the first place. They have to be simple, clear, and understandable so that they are easy to maintain.

================
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
----------------
SouraVX wrote:
> 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!
> I think(at the moment, writing this patch), they are needed.
How are `DW_AT_producer`, `DW_AT_language`, `DW_AT_name` etc. needed to set `MaxVersion`?

> But I guess after D78555, this shouldn't be the problem :)
D78555 is solely about supporting `DWARF64` string offsets tables. It is not expected to have any effect on this patch.

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

https://reviews.llvm.org/D78500