Page MenuHomePhabricator

[lld-macho] handle options -search_paths_first, -search_dylibs_first
ClosedPublic

Authored by gkm on Sep 21 2020, 4:10 PM.

Details

Diff Detail

Event Timeline

gkm created this revision.Sep 21 2020, 4:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2020, 4:10 PM
gkm requested review of this revision.Sep 21 2020, 4:10 PM
gkm updated this revision to Diff 293316.Sep 21 2020, 7:18 PM
  • s/auto/opt::Arg/
gkm updated this revision to Diff 293468.Sep 22 2020, 8:40 AM
  • check args once and save result in Config::searchDylibsFirst
  • split some long command lines in lld/test/MachO/link-search-order.s
int3 added a subscriber: int3.Sep 22 2020, 9:38 AM
int3 added inline comments.
lld/MachO/Driver.cpp
107–111

at some point we should create a LinkerDriver class like in lld-ELF, then we can move driver-only options there

108

this should probably be an if instead of a return

lld/test/MachO/link-search-order.s
19–21

nit: align the right column

gkm updated this revision to Diff 293503.Sep 22 2020, 10:38 AM
  • Fix braino in findLibrary() (consecutive returns: first always shadows second)
  • Refine tests to catch the braino
gkm marked an inline comment as done.Sep 22 2020, 10:48 AM
gkm added inline comments.
lld/test/MachO/link-search-order.s
19–21

I already submitted a major rework of the tests before seeing your comments. This open-source phabricator does not keep a history of diff revisions, so I can't see the original version on which you commented. Have a look at the latest and tell me if the right column still needs attention.

int3 accepted this revision.Sep 23 2020, 10:41 AM

This open-source phabricator does not keep a history of diff revisions

You can see previous diffs by clicking on the link in e.g. gkm updated this revision to Diff 293468.

lld/test/MachO/link-search-order.s
15–55

I was talking about aligning these

This revision is now accepted and ready to land.Sep 23 2020, 10:41 AM
This revision was landed with ongoing or failed builds.Sep 23 2020, 2:57 PM
This revision was automatically updated to reflect the committed changes.

This seems to break tests everywhere, eg http://45.33.8.238/linux/28628/step_11.txt

Ptal, and revert if it takes a while to fix.

gkm added a comment.Sep 23 2020, 7:03 PM

@thakis, the failure only affects incremental build & test. Everything is good on a clean tree. What do you advise now?

Sounds like that puts it in the "takes a while to figure out" category, so revert for now?

What exactly is the problem?

int3 added a comment.Sep 24 2020, 5:00 AM

@thakis I think @gkm's question is around whether incremental builds need to be supported, since the public LLVM buildbots all seem to test clean builds only.

All devs do incremental builds. Of course incremental builds should work.

I went ahead and fixed this in https://github.com/llvm/llvm-project/commit/0389eff4047a74bb1ba6c0603c9002b5c73b203e

Going forward, please don't leave stuff broken overnight.

gkm added a comment.Sep 24 2020, 11:08 PM

All devs do incremental builds. Of course incremental builds should work.

I went ahead and fixed this in https://github.com/llvm/llvm-project/commit/0389eff4047a74bb1ba6c0603c9002b5c73b203e

Going forward, please don't leave stuff broken overnight.

Thanks very much for rescuing a n00b. I'll do better next time. 🤔