[PATCH] D88151: [LLD][ELF] Fix inconsistencies with ICF equality class

[Andrew Ng via Phabricator via llvm-commits]

andrewng added inline comments.


================
Comment at: lld/ELF/ICF.cpp:225
+    // Set MSB to 1 to avoid equality with "unique" IDs.
+    uint32_t eqClass = mid | (1U << 31);
     for (size_t i = begin; i < mid; ++i)
----------------
MaskRay wrote:
> There is indeed one existing `1U << 31` ... but there is no accompanying `& 0x7fffffff` I would expect.. So I start to wonder its merit..
Setting the MSB avoids this `eqClass` being equal to an `eqClass` that was initialized via `uniqueId` on line 476. This was one of my concerns regarding a possible incorrect equality. However, this all does assume that both the number of sections in `sections` and `uniqueId` are less than `(1U << 31)`.

The setting of the MSB elsewhere is to avoid a hash collision which is probably useful but not as important.


================
Comment at: lld/ELF/ICF.cpp:476
     part.ehFrame->iterateFDEWithLSDA<ELFT>(
-        [&](InputSection &s) { s.eqClass[0] = ++uniqueId; });
+        [&](InputSection &s) { s.eqClass[0] = s.eqClass[1] = ++uniqueId; });
 
----------------
MaskRay wrote:
> This is unneeded. combineRelocHashes updates eqClass[1]
AFAIK, `combineRelocHashes` only acts on `sections` and the sections that have had `eqClass[0]` assigned via `uniqueId` will not be in `sections` (see line 481).


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

https://reviews.llvm.org/D88151