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

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:154-155
+
+      // FIXME: Add support for DWARF64.
+      int StrOffsetBases = 8;
+
----------------
dblaikie wrote:
> Is there an API you could call to provide this answer, rather than hardcoding it? (like a MacroHeader structure that tells you the size based on DWARF32/64 enum - you could hardcode passing the DWARF32 enum value to such an API for now, but at least calling the API rather than hardcoding it would be good)
Primary reason why this value is hard-coded here is because in a `dwo` object there can be only one contribution to `debug_str_offets` section. Hence `StrOffsetsBase` can be safely set to 8 bytes(Header Size).

Should we leave it as it ?? is so that iff `strx` form got supported in a non-dwo object too, then a function can should set up this correctly(discussed in next comment) with exception that if it's a `dwo` object set it up as 8/16 bytes according to DWARF32/64.


================
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);
----------------
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.





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78500