This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Filter TAPI re-exports by target
ClosedPublic

Authored by int3 on Mar 3 2021, 8:44 AM.

Details

Reviewers
oontvoo
Group Reviewers
Restricted Project
Commits
rG55a32812fa5e: [lld-macho] Filter TAPI re-exports by target
Summary

Previously, we were loading re-exports without checking whether
they were compatible with our target. Prior to D97209: [lld-macho] Check for arch compatibility when loading ObjFiles and TBDs, it meant that
we were defining dylib symbols that were invalid -- usually a silent
failure unless our binary actually used them. D97209 exposed this as an
explicit error.

Along the way, I've extended our TAPI compatibility check to cover the
platform as well, instead of just checking the arch. To this end, I've
replaced MachO::Architecture with MachO::Target in our Config struct.

Diff Detail

Event Timeline

int3 created this revision.Mar 3 2021, 8:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2021, 8:44 AM
int3 requested review of this revision.Mar 3 2021, 8:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2021, 8:44 AM
oontvoo added a subscriber: oontvoo.Mar 3 2021, 9:12 AM
oontvoo added inline comments.
lld/MachO/Config.h
86

nit: could this be called makeTarget? target() alone sounds like an accessor, which it technically isn't.

Otherwise (keeping the name), can it be cached/shared? How many time does it need to create a target?

(It seems from the usages below, we only need it for comparison, so it seems safe to do that to me)

int3 updated this revision to Diff 327886.Mar 3 2021, 12:22 PM
int3 edited the summary of this revision. (Show Details)

construct Target just once

int3 marked an inline comment as done.Mar 3 2021, 12:25 PM
int3 added inline comments.
lld/MachO/Config.h
86

Making it sound like an accessor was intentional ;) Target's constructor is basically a no-op, so target() was just constructing a pair of two enums, which is super cheap. But I get that it looks misleading / surprising at first glance, so I've cleaned it up.

lld/MachO/Driver.cpp
586

I moved the definition of this function down since it now invokes parsePlatformVersion

oontvoo accepted this revision.Mar 3 2021, 3:09 PM

LGTM

This revision is now accepted and ready to land.Mar 3 2021, 3:09 PM
This revision was landed with ongoing or failed builds.Mar 4 2021, 11:37 AM
This revision was automatically updated to reflect the committed changes.
int3 marked an inline comment as done.
lld/test/MachO/Inputs/libStubLink.tbd