This is an archive of the discontinued LLVM Phabricator instance.

[LLD][ELF] Fix inconsistencies with ICF equality class
AbandonedPublic

Authored by andrewng on Sep 23 2020, 7:50 AM.

Details

Summary

There appears to be an inconsistent approach to the setting of the
InputSection eqClass in ICF which could cause incorrect equivalence
class equality.

It would appear that this has not caused any issues, but making it
consistent and therefore more deterministic feels safer.

Diff Detail

Event Timeline

andrewng created this revision.Sep 23 2020, 7:50 AM
andrewng requested review of this revision.Sep 23 2020, 7:50 AM
andrewng added a subscriber: gbreynoo.

I may be missing something about how the ICF code works, but whilst looking at a separate ICF performance issue, I noticed these oddities in the code. I haven't actually seen any issues myself but reading through the code, it feels like there could be issues. Although I suspect they would be highly unlikely or perhaps not possible in actual practice.

MaskRay added inline comments.Sep 23 2020, 8:44 AM
lld/ELF/ICF.cpp
225

There is indeed one existing 1U << 31 ... but there is no accompanying & 0x7fffffff I would expect.. So I start to wonder its merit..

476

This is unneeded. combineRelocHashes updates eqClass[1]

andrewng added inline comments.Sep 23 2020, 9:13 AM
lld/ELF/ICF.cpp
225

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.

476

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).

andrewng abandoned this revision.Oct 5 2020, 6:12 AM

Abandoning due to lack of interest and in favour of D88830 which requires these changes.