This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Refactor archive loading
ClosedPublic

Authored by int3 on Aug 26 2021, 12:12 PM.

Details

Reviewers
gkm
oontvoo
Group Reviewers
Restricted Project
Commits
rG9065fe559119: [lld-macho] Refactor archive loading
Summary

The previous logic was duplicated between symbol-initiated
archive loads versus flag-initiated loads (i.e. -force_load and
-ObjC). This resulted in code duplication as well as redundant work --
we would create Archive instances twice whenever we had one of those
flags; once in getArchiveMembers and again when we constructed the
ArchiveFile.

This was motivated by an upcoming diff where we load archive members
containing ObjC-related symbols before loading those containing
ObjC-related sections, as well as before performing symbol resolution.
Without this refactor, it would be difficult to do that while avoiding
loading the same archive member twice.

Diff Detail

Unit TestsFailed

Event Timeline

int3 created this revision.Aug 26 2021, 12:12 PM
Herald added a project: Restricted Project. · View Herald Transcript
int3 requested review of this revision.Aug 26 2021, 12:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2021, 12:12 PM

LGTM thanks!!!

lld/MachO/Driver.cpp
255

clang-tidy: use auto *file pls.

261–267

We've already done error() on line 262, why do we need to do it again on 266?
(Wouldn't this error on the last error twice?)

283–289

same question here w.r.t erroring twice.

(also the code looks alsmot identical - maybe future clean up could factor them out somehow)

lld/MachO/InputFiles.cpp
1267–1268

could you include the unhandled type in the err msg? (makes it easier to debug when we do get this error)

lld/MachO/InputFiles.h
190

Why not? Just curious

193

Does this want to return a const ref instead?

int3 added inline comments.Aug 26 2021, 1:24 PM
lld/MachO/Driver.cpp
261–267

They're different errors -- one error is from fetching, the other one from trying to get the children

283–289

I guess we could have a helper function that just extracts the children and prints the error, but in general I think this code is ugly because the underlying Archive API is ugly

lld/MachO/InputFiles.h
190

we usually just dump the error to stderr directly via error() or fatal(), so there's no need to return it to the caller

193

yeah we can do that

oontvoo accepted this revision.Aug 26 2021, 1:28 PM

LGTM

lld/MachO/Driver.cpp
261–267

D'oh - different errors, same name. Sorry missed it!

This revision is now accepted and ready to land.Aug 26 2021, 1:28 PM
This revision was landed with ongoing or failed builds.Aug 26 2021, 3:57 PM
This revision was automatically updated to reflect the committed changes.
int3 marked 5 inline comments as done.
thevinster added inline comments.
lld/MachO/InputFiles.cpp
1312

nit: I'm not sure what the standard practice is, but for readability, I feel like it would be more helpful to have this error message be different than L1292. This makes it easier to debug since toMachOString can contain hidden a :.