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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 2 inline comments as done.
MaskRay added inline comments.


================
Comment at: lld/ELF/ICF.cpp:524
+      if (sec->areRelocsRela)
+        handleLSDA(*part.ehFrame, *sec, sec->template relas<ELFT>());
+      else
----------------
grimar wrote:
> MaskRay wrote:
> > 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.
> > 
> It is just strange to see that we drop the `xxHash64` work that was done, it looks excessive.
> 
> Right above we have the following code:
> 
> ```
>     auto *s = cast<InputSection>(sec);
>     if (isEligible(s))
>       sections.push_back(s);`
> ```
> 
> I've not checked, but have you considered something like not putting those sections into `sections` list at all?
> I.e. make them non-eligible for ICF from the begining?
Good idea. Simplified.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84610