Page MenuHomePhabricator

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

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


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

They cause their corresponding libraries / frameworks to be loaded via

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.

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.


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/ for these.

int3 marked an inline comment as done.Sep 23 2020, 8:44 AM
int3 added inline comments.

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


gkm added inline comments.Sep 23 2020, 9:56 AM

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


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

gkm accepted this revision.Sep 23 2020, 2:04 PM
gkm added inline comments.

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

¯\_(ツ)_/¯ 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.