Page MenuHomePhabricator

[lld-macho] Support -weak_lx, -weak_library, -weak_framework
ClosedPublic

Authored by int3 on Sep 18 2020, 12:19 PM.

Details

Reviewers
gkm
Group Reviewers
Restricted Project
Commits
rG98f03908d07d: [lld-macho] Support -weak_lx, -weak_library, -weak_framework
Summary

They cause their corresponding libraries / frameworks to be loaded via
LC_LOAD_WEAK_DYLIB instead of LC_LOAD_DYLIB.

Diff Detail

Event Timeline

int3 created this revision.Sep 18 2020, 12:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2020, 12:19 PM
int3 requested review of this revision.Sep 18 2020, 12:19 PM
gkm added a subscriber: gkm.Sep 22 2020, 6:05 PM
gkm added inline comments.
lld/MachO/Driver.cpp
603–611

IMO, there is no benefit in combining these cases, and a deficit in clarity.
By contrast, the other two OPT_weak_l and OPT_weak_framework do benefit from combining, since they have surrounding context that need not be duplicated.

650–654

How about setting the forceWeakImport flag immediately after construction? Perhaps create void addWeakFile(StringRef path) and/or void addFile(StringRef path, bool isWeak) APIs around void addFile(StringRef path) that sets the flag when addFlag() returns a DylibFile. That way you don't need weakImportPaths.

gkm added a comment.Sep 23 2020, 12:05 AM

Be sure to remove Flags<[HelpHidden]> from ldd/MachO/Options.td for these.

int3 marked an inline comment as done.Sep 23 2020, 8:44 AM
int3 added inline comments.
lld/MachO/Driver.cpp
650–654

I agree this is kind of janky, but I think having an isWeak parameter that only has an effect when the resulting path contains a DylibFile is also kind of ugly...

int3 updated this revision to Diff 293757.Sep 23 2020, 8:46 AM

helphidden

gkm added inline comments.Sep 23 2020, 9:56 AM
lld/MachO/Driver.cpp
618–626

How about something like this ...
Also change addFile() to return its newly-constructed InputFile*

int3 updated this revision to Diff 293806.Sep 23 2020, 11:09 AM
int3 marked an inline comment as done.

have addFile return the new file

lld/MachO/Driver.cpp
618–626

that was cleaner than I'd anticipated it being. thanks!

gkm accepted this revision.Sep 23 2020, 2:04 PM
gkm added inline comments.
lld/MachO/Driver.cpp
608

I always find explicit nullptr-inequality checks to be overly noisy. I just looked and found that there are only three instances of != nullptr in all of lld/MachO/*.{h,cpp}, which IMO is at least two too many. The third is semi-legit for highlighting the boolean nature of pointer-to-something vs. pointer-to-nothing:

./UnwindInfoSection.cpp:  return (compactUnwindSection != nullptr);

If you don't feel so strongly about it, then please allow me to impose my will regarding this stylistic nit. If you do feel strongly, then I bet you could find a ton of places to add != nullptr. We could even start an edit war! 😆

This revision is now accepted and ready to land.Sep 23 2020, 2:04 PM
int3 added inline comments.Sep 23 2020, 2:17 PM
lld/MachO/Driver.cpp
608

¯\_(ツ)_/¯ I mean, I think != nullptr is in line with "explicit over implicit" (yeah I know this isn't Python, but I like to carry the zen of Python wherever I go)... but it's a minor issue in the scheme of things

This revision was automatically updated to reflect the committed changes.