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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/ELF/EhFrame.cpp:215
+      skipAugP();
+    else if (c == 'L')
+      return true;
----------------
I'd put this first as it is what you are looking for:

```
for (char c : aug) {
  if (c == 'L')
    return true;
...
}
```


================
Comment at: lld/ELF/ICF.cpp:468
+// with PC-relative encoding), we will lose folding opportunity but such cases
+// are rare.
+//
----------------
Did you perform a test of a some project that uses exceptions? I wonder if it is really that rare.


================
Comment at: lld/ELF/ICF.cpp:475
+                           ArrayRef<RelTy> rels) {
+  llvm::DenseMap<size_t, bool> offsetToBool;
+  uint32_t uniqueId = 0;
----------------
It is probably more natural for this to be `llvm::DenseSet<size_t>`?

E.g:

```
llvm::DenseSet<size_t> ciesWithLDSA;
...
    if (id == 0 && hasLSDA(piece)) {
      ciesWithLDSA.insert(offset);
      continue;
    }
    uint32_t cieOffset = offset + 4 - id;
    if (!ciesWithLDSA.contains(cieOffset))
      continue;
...
```


================
Comment at: lld/ELF/ICF.cpp:483
+    size_t offset = piece.inputOff;
+    uint32_t id = support::endian::read32<ELFT::TargetEndianness>(
+        piece.data().data() + 4);
----------------
The Length can take either 4 or 12 bytes (extended length). Should this be handled?


================
Comment at: lld/ELF/ICF.cpp:524
+      if (sec->areRelocsRela)
+        handleLSDA(*part.ehFrame, *sec, sec->template relas<ELFT>());
+      else
----------------
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?


================
Comment at: lld/ELF/SyntheticSections.cpp:374
+// There is one FDE per function. Returns a non-null pointer if a given FDE
 // points to a live function.
 template <class ELFT, class RelTy>
----------------
I think the comment should describe the `Defined` symbol returned.
A non-null pointer sounds like it can be any `!nullptr`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84610