[PATCH] D84610: [ELF] --icf: don't fold text sections with LSDA

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/EhFrame.cpp:215
+      skipAugP();
+    else if (c == 'L')
+      return true;
----------------
grimar wrote:
> I'd put this first as it is what you are looking for:
> 
> ```
> for (char c : aug) {
>   if (c == 'L')
>     return true;
> ...
> }
> ```
Emm. I tend to prefer the order where compilers usually generate the augmentation string.


================
Comment at: lld/ELF/ICF.cpp:483
+    size_t offset = piece.inputOff;
+    uint32_t id = support::endian::read32<ELFT::TargetEndianness>(
+        piece.data().data() + 4);
----------------
grimar wrote:
> The Length can take either 4 or 12 bytes (extended length). Should this be handled?
The extended length format .eh_frame is not supported by MC (lib/MC/MCDwarf.cpp:EmitFDE).

The previous function does not handle the extended length as well.

Personally I think it is rarer than DWARF64

"A 8 byte unsigned value indicating the length in bytes of the CIE structure, not
including the Length and Extended Length fields themselves" It is hard to get a single CIE structure larger than 4GiB. This is different from DWARF64 where the FDE field is an absolute offset instead of a PC-relative offset (so the total size of .debug_frame matters)


================
Comment at: lld/ELF/ICF.cpp:524
+      if (sec->areRelocsRela)
+        handleLSDA(*part.ehFrame, *sec, sec->template relas<ELFT>());
+      else
----------------
grimar wrote:
> It's a bit strange to see that `eqClass[0]` is set to `xxHash64(s->data()` right above,
> but `handleLSDA` might override the value to 1,2...N?
> 
> Should the hash be combined into, e.g. like `combineRelocHashes` does?
Not necessary. By assigning 1,2,...,N, we don't actually want such sections to be folded into others. The combine operation is redundant.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84610