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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

> This results in a 4.35% size decrease

Sweet.



================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1063
       Info.MangledName = StringEntry;
-
+    if (AttrSpec.Form == dwarf::DW_FORM_strx ||
+        (AttrSpec.Form >= dwarf::DW_FORM_strx1 &&
----------------
avl wrote:
> I would suggest to use DwarfVersion as a sign of which resulting form would be used:
> 
> ```
> if (DwarfVersion >= 5) {
>       // Switch everything to DW_FORM_strx strings.
>       auto StringOffsetIndex =
>           StringOffsetPool.getValueIndex(StringEntry.getOffset());
>       return Die
>           .addValue(DIEAlloc, AttrSpec.Attr,
>                     dwarf::DW_FORM_strx, DIEInteger(StringOffsetIndex))
>           ->sizeOf(U.getFormParams());
> }
> 
>     // Switch everything to DW_FORM_strp strings.
>     return Die
>           .addValue(DIEAlloc, AttrSpec.Attr,
>                     dwarf::DW_FORM_strp, DIEInteger(StringEntry.getOffset()))
>           ->sizeOf(U.getFormParams());
> 
> ```
> 
> This allows to save more space if original CU is of 5 version but uses DW_FORM_strp forms. Also it would allow to simplify patch by removing StrxFoundInCU and AttrStrOffsetBaseSeen flags. 
> 
> Though we would need to pay attention to the case when original CU of 5 dwarf version does not have  DW_AT_str_offsets_base because it uses DW_FORM_strp forms. We would need to add DW_AT_str_offsets_base in that case.
That seems reasonable — if makes the patch simpler.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1068
+          StringOffsetPool.getValueIndex(StringEntry.getOffset());
+      // Mark StrxFoundInCU as true if a DW_FORM_strx* is found in any Die of a
+      // CU, use it to throw an error if a DW_AT_str_offsets_base is not found
----------------
Very nitpicky nit: it's `DIE` because it's an abbreviation for Debug Info Entry ;-)


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:2025
+  if (StrxFoundInCU)
+    llvm_unreachable("Didn't find a DW_AT_str_offsets_base in cloned DIE!");
+}
----------------
Just double-checking that this is really unreachable, even with malformed input?


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:2592
+      if (DwarfVersion >= 5)
+        patchStrOffsetsBase(*CurrentUnit->getOutputUnitDIE(), DIEInteger(8), StrxFoundInCU);
     }
----------------
If I'm reading the DWARF spec correctly, isn't the header size 4+2+2 in DWARF32 and 4+8+2+2 in DWARF64?


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

https://reviews.llvm.org/D157036