[lld] [lld][ELF] Merge equivalent symbols found during ICF (PR #139493)

[Fangrui Song via llvm-commits]

MaskRay wrote:

Let'me rephrase the problem. When we have  (https://github.com/llvm/llvm-project/pull/131630#issuecomment-2770780719)

> * a Section A1, with a relocation on a symbol S1, where S1 is at offset K in section B1.
> * a Section A2, with a relocation on a symbol S2, where S2 is at offset K in section B2.
> 
> Currently, ICF mostly assumes that if you can merge B1 and B2, then you can also merge A1 and A2, without worrying about the symbol's identity.

This aggressive section merging, without accounting for symbol identity, causes correctness issues in the AArch64 MachineOutliner case (#129122).
Specifically, **when the relocation type is R_AARCH64_ADR_GOT_PAGE, and sections B1 and B2 are merged while keeping symbols S1 and S2 separate, sections A1 and A2 must not be merged to maintain correctness.**

To address this, we propose merging symbols S1 and S2 (by redirecting their `Symbol *` pointers to a single leader `Symbol *`) when their respective sections B1 and B2 are merged.
This merging process involves redirecting the `Symbol *` entry in the file's symbol table.
However, due to the local-before-non-local symbol table requirement, merging a local symbol with a non-local symbol is prohibited.

To resolve this, **the proposed solution in this pull request is to**:

* When scanning A1/A2, try merging referenced symbols S1/S2.
* Prevent A1/A2 merge if the relocation is "non-trivial" and any of S1/S2 is local. (As S1/S2 will be separate, sections A1 and A2 must not be merged)

---

* The patch uses `!isGlobal()` (STB_GLOBAL), which should switch to `isLocal()`. STB_WEAK/STB_GNU_UNIQUE should be handled the same way as STB_GLOBAL.
* Symbol merging is performed by scanning referended symbols (`getRelocTargetSyms`). Should we build a section-to-symbol map by scanning the file symbol table instead? In general, I want to reduce `getRelocTargetSym` calls.
* The `da->value + addA == db->value + addB` condition in `constantEq` is not well-motivated (I doubt there is any benefit). Can probably switch to `da->value == db->value && addA == addB` and remove a `isTrivialRelocation` call.
* We should not move `OffsetGetter` and `needsGot` from Relocations.cpp. Most targets' `getRelExpr` don't need the `loc` argument. I think we can pass a fake loc ensuring validity of `loc[-1]` (used by X86.cpp) and `loc[3]` (used by LoongArch). Then we don't need `OffsetGetter` at all.

The proposed changes add considerable complexity, and I'm still uncertain about incorporating them into ICF.cpp. That said, thank you for creating the test cases! If I were designing an ABI, **I would simply disallow GOT code sequences that span multiple sections**.


https://github.com/llvm/llvm-project/pull/139493