[PATCH] D78500: [DWARF5]:Added support for .debug_macro.dwo section in llvm-dwarfdump
Igor Kudrin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 22 23:18:41 PDT 2020
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
More information about the llvm-commits
mailing list