[lld] [lld-macho] Choose ICF root function deterministically based on symbol names (PR #158157)

[Ellis Hoag via llvm-commits]

================
@@ -408,11 +408,35 @@ void ICF::run() {
         // When using safe_thunks, ensure that we first sort by icfEqClass and
         // then by keepUnique (descending). This guarantees that within an
         // equivalence class, the keepUnique inputs are always first.
-        if (config->icfLevel == ICFLevel::safe_thunks)
-          if (a->icfEqClass[0] == b->icfEqClass[0])
-            return a->keepUnique > b->keepUnique;
+        if (a->icfEqClass[0] == b->icfEqClass[0]) {
+          if (config->icfLevel == ICFLevel::safe_thunks) {
+            if (a->keepUnique != b->keepUnique)
+              return a->keepUnique > b->keepUnique;
+          }
+          // Within each equivalence class, sort by symbol name for determinism.
+          // If a section has an external symbol at offset 0, use that name.
+          // If either section doesn't have such a symbol, retain the original
+          // order.
+          auto getExternalSymbolName =
+              [](const ConcatInputSection *isec) -> std::optional<StringRef> {
+            for (const Symbol *sym : isec->symbols) {
+              if (const auto *d = dyn_cast<Defined>(sym)) {
+                if (d->value == 0 && d->isExternal())
+                  return d->getName();
+              }
+            }
+            return std::nullopt;
+          };
+          auto nameA = getExternalSymbolName(a);
+          auto nameB = getExternalSymbolName(b);
+          // For determinism, define a strict ordering only when both sections
+          // have a primary symbol. Otherwise, they are considered equivalent
+          // (the expression returns false), preserving the stable sort order.
+          return nameA && nameB && *nameA < *nameB;
----------------
ellishg wrote:

I don't think this is a valid ordering, although it's complicated to prove. https://en.cppreference.com/w/cpp/named_req/Compare.html

Consider three symbols A, B, C with names "A", "B", and `std::nullopt`.
Since any comparison with `C` is false, we know `equiv(A, C) == equiv(B, C) == false`. But from the requirements above, it must be that `equiv(A, B)` is true, with is not that case.

I think the correct thing to do is to simply `return nameA < nameB` without checking whether they have values. `std::optional` basically treats `std::nullopt` as the smallest possible value. https://en.cppreference.com/w/cpp/utility/optional/operator_cmp.html

I'm am not positive this will give the behavior you want though. It will basically order symbols by their name if they exist, then push all the symbols without names to the end, in a stable order.

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