This is an archive of the discontinued LLVM Phabricator instance.

[ELF] --icf: do not fold preemptible symbols
ClosedPublic

Authored by MaskRay on Dec 7 2019, 12:55 AM.

Details

Summary

Fixes PR44124.

A preemptible symbol may refer to a different definition at runtime.
When comparing a pair of relocations, if they refer to different
symbols, and either symbol is preemptible, the two containing sections
should be considered different.

gold has a similar rule https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=ce97fa81e0c46d216b80b143ad8c02fff6906fef

Diff Detail

Event Timeline

MaskRay created this revision.Dec 7 2019, 12:55 AM
MaskRay marked an inline comment as done.Dec 7 2019, 9:00 AM
MaskRay added inline comments.
lld/ELF/ICF.cpp
262

I may place a comment that replicates the patch summary here

When comparing a pair of relocations, if they refer to different symbols, and either symbol is preemptible, the containing sections should be considered different.

If you have a wording suggestion, please tell me:)

Generally LG.

lld/ELF/Symbols.h
557

use sym for consistency with the definition of maybeWarnUnorderableSymbol?

lld/test/ELF/icf-preemptible.s
20

Not sure LEAF and NONLEAF are clear to me.
I have no suggestions about how to name them better though.
It seems a bit hard to invent good names when checks are mixed.
If they were separated you could use just "DSO" and "EXE" probably.
May be others will have any ideas.

peter.smith added inline comments.Dec 9 2019, 1:48 AM
lld/ELF/ICF.cpp
262

Only addition I'd make is a brief comment on why they should be different.

When comparing a pair of relocations, if they refer to different symbols, and either symbol is preemptible, the containing sections should be considered different. This is because even if the sections are identical in this DSO, they may not be after preemption.
lld/test/ELF/icf-preemptible.s
20

I'm clear about the meaning of LEAF and NONLEAF, although I suppose we could add a comment to make it clearer. Having said that it would be worth exploring George's suggestion to have a DSO and EXE case, it would be easier to map to the definitions of f and g as well.

MaskRay updated this revision to Diff 232895.Dec 9 2019, 10:38 AM
MaskRay marked 4 inline comments as done.

Address review comments

grimar accepted this revision.Dec 10 2019, 12:59 AM

LGTM

This revision is now accepted and ready to land.Dec 10 2019, 12:59 AM
This revision was automatically updated to reflect the committed changes.
ruiu added inline comments.Dec 10 2019, 9:27 PM
lld/ELF/Driver.cpp
1992–1996

This seems like a logic that should belong to ICF, so maybe you can move this to doICF, so that this logic is hidden from others?