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

[James Y Knight via llvm-commits]

================
@@ -535,14 +557,35 @@ template <class ELFT> void ICF<ELFT>::run() {
   auto print = [&ctx = ctx]() -> ELFSyncStream {
     return {ctx, ctx.arg.printIcfSections ? DiagLevel::Msg : DiagLevel::None};
   };
+
+  DenseMap<Symbol *, Symbol *> symbolMap;
   // Merge sections by the equivalence class.
+  // Merge symbols identified as equivalent during ICF
   forEachClassRange(0, sections.size(), [&](size_t begin, size_t end) {
     if (end - begin == 1)
       return;
     print() << "selected section " << sections[begin];
+    SmallVector<Symbol *> syms = getRelocTargetSyms<ELFT>(sections[begin]);
     for (size_t i = begin + 1; i < end; ++i) {
       print() << "  removing identical section " << sections[i];
       sections[begin]->replace(sections[i]);
+      SmallVector<Symbol *> replacedSyms =
+          getRelocTargetSyms<ELFT>(sections[i]);
+      assert(syms.size() == replacedSyms.size() &&
+             "Should have same number of syms!");
+      for (size_t i = 0; i < syms.size(); i++) {
+        if (syms[i] == replacedSyms[i] || !syms[i]->isGlobal() ||
----------------
jyknight wrote:

Since you're checking isGlobal only here, and not in variableEq, doesn't that mean that any non-trivial relocation (such as GOT) which references a local symbol will remain broken? Maybe ELF producers don't _need_ to create a GOT reloc pointing to a local symbol, but it's not invalid to do so.

We can't (at least not right now) get rid of the isGlobal check, because we cannot actually canonicalize local symbols across TUs, since code doing loops over `getLocalSymbols()` for each file in `ctx.objectFiles` expects to see no duplicates. (which was a bug in my original version).

So I think we need to add a check in variableEq, `if((!sa.isGlobal() || !sb.isGlobal()) && !ra->"isTrivialRelocType"()) return false;`. (I made up "isTrivialRelocType"; do we have a relevant predicate already that would determine if a relocation has semantics beyond just the offset?)

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