[PATCH] D157036: Emit a .debug_str_offsets section with dsymutil to support DW_FORM_strx in dsymutil.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

Thank you for the update. Would you mind creating the test case for DW_AT_str_offsets_base and debug_str_offsets, please?(similar to dwarf5-addr_base.test, i.e. several compile units, update/no-update). And, probably, add checks for strings forms into the  dwarf5-dwarf4-combination-macho.test(that dwarf5 has all required new forms, and dwarf4 still uses old forms), please.



================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:665
+    DebugDieValuePool AddrPool;
+    bool AttrStrOffsetBaseSeen = false;
 
----------------
there is AttributesInfo structure which keeps exactly that kind of info. it would be good to make AttrStrOffsetBaseSeen to be member of AttributesInfo.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:878
+  /// to emit a .debug_str_offsets section.
+  bool Dwarf5CUSeen = false;
+
----------------
the DWARFLinkerOptions::TargetDWARFVersion version could be used instead of this additional flag. It would be set into the version 5 in case any DWARFv5 CU seen.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFStreamer.h:85
+  /// Emit the debug string offset table described by \p StringOffsets into the
+  /// .debug_string_offset table.
+  void emitStringOffsets(const SmallVector<uint64_t> &StringOffset) override;
----------------



================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1845
+    OutOffset += 4;
+  }
+
----------------
It should be added to only DW_TAG_compile_unit DIEs.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:2015
 
+static void patchStrOffsetsBase(DIE &Die, DIEInteger Offset) {
+  for (auto &V : Die.values())
----------------
usage of patchStrOffsetsBase looks a bit reversed. That patch* routines are used when the value is not known at the moment the input DIE is clonned. In this concrete case the offset value is known. It would be more straightforward if proper value of DW_AT_str_offsets_base would be set in cloneScalarAttribute routine(and patchStrOffsetsBase would be removed):


```
unsigned DWARFLinker::DIECloner::cloneScalarAttribute()
...
if (AttrSpec.Form == dwarf::DW_AT_str_offsets_base) {
    // DWARFLinker generates common .debug_str_offsets table used for all compile units.
    // The offset to the common .debug_str_offsets table is 8 on DWARF32.
    AttrSpec.AttrStrOffsetBaseSeen = true;
    return Die.addValue(DIEAlloc, dwarf::DW_AT_str_offsets_base,
                  dwarf::DW_FORM_sec_offset, DIEInteger(8))->sizeOf(U.getFormParams())
}
...
```


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:2944
       TheDwarfEmitter->emitStrings(DebugStrPool);
+      if (Dwarf5CUSeen && !StringOffsetPool.DieValues.empty())
+        TheDwarfEmitter->emitStringOffsets(StringOffsetPool.DieValues);
----------------
It would probably look cleaner if checking for above conditions would be hidden inside emitStringOffsets like in emitDebugAddrSection.


================
Comment at: llvm/lib/DWARFLinker/DWARFStreamer.cpp:262
+/// Emit the debug string offset table described by \p StringOffsets into the
+/// .debug_string_offset table.
+void DwarfStreamer::emitStringOffsets(
----------------



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

https://reviews.llvm.org/D157036