This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Simplify archive loading logic
ClosedPublic

Authored by int3 on Jul 19 2022, 6:21 PM.

Details

Reviewers
PRESIDENT810
thakis
Group Reviewers
Restricted Project
Commits
rG87ce7b41d82a: [lld-macho] Simplify archive loading logic
Summary

This is a follow-on to D129556: [lld-macho] Fix loading same libraries from both LC_LINKER_OPTION and command line. I've refactored the code such that
addFile() no longer needs to take an extra parameter. Additionally,
the "do we force-load or not" policy logic is now fully contained within
addFile, instead of being split between addFile and
parseLCLinkerOptions. This also allows us to move the ForceLoad (now
LoadType) enum out of the header file.

Additionally, we can now correctly report loads induced by
LC_LINKER_OPTION in our -why_load output.

I've also added another test to check that CLI library non-force-loads
take precedence over LC_LINKER_OPTION + -force_load_swift_libs. (The
existing logic is correct, just untested.)

Diff Detail

Event Timeline

int3 created this revision.Jul 19 2022, 6:21 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 19 2022, 6:21 PM
int3 requested review of this revision.Jul 19 2022, 6:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2022, 6:21 PM
thakis added a subscriber: thakis.Jul 19 2022, 6:26 PM
thakis added inline comments.
lld/MachO/Driver.cpp
296–299

!isCommandLineLoad instead of == false

304

Why && forceLoadSwift && path::filename(path).startswith("libswift")? Does this in LC_LINKER_OPTIONS only apply to swift archives?

int3 marked an inline comment as done.Jul 19 2022, 6:28 PM
int3 added inline comments.
lld/MachO/Driver.cpp
296–299

d'oh, of course

304

yup

int3 updated this revision to Diff 446007.Jul 19 2022, 6:30 PM
int3 marked an inline comment as done.

update

int3 marked an inline comment as done.Jul 19 2022, 6:30 PM
thakis accepted this revision.Jul 19 2022, 6:32 PM

LG

lld/MachO/Driver.cpp
254

Maybe say "only applies to swift files" in the comment here, or put "Swift" in the constant's name then?

This revision is now accepted and ready to land.Jul 19 2022, 6:32 PM
int3 added inline comments.Jul 19 2022, 6:38 PM
lld/MachO/Driver.cpp
254

the swift-force-loading only applies to libraries though, not frameworks, but we pass this enum for both library and framework loads

I can add a comment to Config::forceLoadSwift though

int3 added inline comments.Jul 19 2022, 6:40 PM
lld/MachO/Driver.cpp
254

also this enum is passed regardless of whether we are doing a force-load. LoadType just tells us what induced the load, not *how* the load should be performed (i.e. forced or not). I will add that in a comment

int3 updated this revision to Diff 446013.Jul 19 2022, 6:52 PM
int3 edited the summary of this revision. (Show Details)

more comments and another test

This revision was landed with ongoing or failed builds.Jul 19 2022, 6:56 PM
This revision was automatically updated to reflect the committed changes.