Page MenuHomePhabricator

[lld-macho] Basic support for linkage and visibility attributes in LTO
ClosedPublic

Authored by int3 on Jan 8 2021, 2:16 PM.

Details

Summary

When parsing bitcode, convert LTO Symbols to LLD Symbols in order to perform
resolution. The "winning" symbol will then be marked as Prevailing at LTO
compilation time. This is similar to what the other LLD ports do.

This change allows us to handle linkonce symbols correctly, and to deal with
duplicate bitcode symbols gracefully. Previously, both scenarios would result in
an assertion failure inside the LTO code, complaining that multiple Prevailing
definitions are not allowed.

While at it, I also added basic logic around visibility. We don't do anything
useful with it yet, but we do check that its value is valid. LLD-ELF appears to
use it only to set FinalDefinitionInLinkageUnit for LTO, which I think is just a
performance optimization.

From my local experimentation, the linker itself doesn't seem to do anything
differently when encountering linkonce / linkonce_odr / weak / weak_odr. So I've
only written a test for one of them. LLD-ELF has more, but they seem to mostly
be testing the intermediate bitcode output of their LTO backend...? I'm far from
an expert here though, so I might very well be missing things.

Diff Detail

Event Timeline

int3 created this revision.Jan 8 2021, 2:16 PM
int3 requested review of this revision.Jan 8 2021, 2:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2021, 2:16 PM
int3 updated this revision to Diff 315522.Jan 8 2021, 2:29 PM

rebase with stacked diff

The change to select the prevailing seems reasonable to me, but I'm not terribly familiar with some of the lld internals that this touches. Probably best reviewed by @pcc who implemented some of the ELF lld/ELF support, or @MaskRay.

@pcc, @MaskRay, would you like to have a look before I land this?

Herald added a project: Restricted Project. · View Herald TranscriptMon, Feb 8, 6:19 PM

While at it, I also added basic logic around visibility. We don't do anything useful with it yet, but we do check that its value is valid. LLD-ELF appears to use it only to set FinalDefinitionInLinkageUnit for LTO, which I think is just a performance optimization.

D92900 may have some interesting tests.

smeenai accepted this revision.Wed, Feb 24, 2:47 PM
smeenai added a subscriber: smeenai.

LGTM

This revision is now accepted and ready to land.Wed, Feb 24, 2:47 PM
int3 added a comment.Wed, Feb 24, 2:49 PM

@MaskRay thanks for the pointer! I'll try and crib from those tests when I address FinalDefinitionInLinkageUnit in a future diff.

MaskRay accepted this revision.Wed, Feb 24, 2:53 PM