This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Re-root absolute input file paths if -syslibroot is specified
ClosedPublic

Authored by int3 on Apr 8 2021, 3:35 PM.

Details

Diff Detail

Unit TestsFailed

Event Timeline

int3 created this revision.Apr 8 2021, 3:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2021, 3:35 PM
int3 requested review of this revision.Apr 8 2021, 3:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2021, 3:35 PM
int3 planned changes to this revision.Apr 8 2021, 3:39 PM
int3 updated this revision to Diff 336246.Apr 8 2021, 3:41 PM

update

int3 added inline comments.Apr 8 2021, 3:42 PM
lld/MachO/Driver.cpp
121

Since we may return path; from this function, having it return an StringRef instead of an std::string is natural. I therefore modified findPathCombination to return an Optional<StringRef> as well.

For uniformity, findFramework should be modified similarly; I'll do it in a separate commit to avoid cluttering this diff.

lld/test/MachO/dependency-info.s
23

we now attempt to search for a bunch of rerooted paths, so there are more not-found entries

int3 updated this revision to Diff 336262.Apr 8 2021, 4:26 PM

try to get test passing on windows

Harbormaster completed remote builds in B98015: Diff 336492.
int3 updated this revision to Diff 336554.Apr 9 2021, 1:21 PM

I give up; disabling the test on Windows

gkm added a subscriber: gkm.Apr 9 2021, 4:03 PM
gkm added inline comments.
lld/MachO/Driver.cpp
80

What does \p mean here in this comment?

lld/test/MachO/reroot-path.s
27–28

These don't test what you want, since they succeed even without -syslibroot:

  • PASS without -syslibroot %t, since %t/%:t/bar.a is a real path, so the syslibroot is unused
  • PASS with -syslibroot %t and s/%t/%:t/bar.a/%t/bar.a/ for a syslibroot-relative path
  • FAIL (as expected) without -syslibroot %t and s/%t/%:t/bar.a/%t/bar.a/, since syslibroot is needed to reach the syslibroot-relative path
29–30

Same behavior here for libbar.dylib vs. bar.a

31–33

Same

gkm added inline comments.Apr 9 2021, 4:46 PM
lld/test/MachO/reroot-path.s
27–28

I believe the second case is what you want: with -syslibroot %t and s/%t/%:t/bar.a/%t/bar.a/

int3 planned changes to this revision.Apr 9 2021, 4:46 PM
int3 marked 3 inline comments as done.Apr 12 2021, 6:28 PM
int3 added inline comments.
lld/MachO/Driver.cpp
80

Parameter. It's used by Doxygen: https://www.doxygen.nl/manual/commands.html#cmdp

lld/test/MachO/reroot-path.s
27–28

yea good catch. I'll check the actual path accessed with -t.

int3 updated this revision to Diff 337016.Apr 12 2021, 6:28 PM
int3 marked 2 inline comments as done.

update

gkm added inline comments.Apr 12 2021, 8:08 PM
lld/test/MachO/reroot-path.s
26–29

Don't we also want to test with %t/bar.a and %t/libbar.dylib to demonstrate that -syslibroot %t was honored %t prepended to find the file?

int3 marked an inline comment as done.Apr 12 2021, 8:13 PM
int3 added inline comments.
lld/test/MachO/reroot-path.s
26–29

oops, yeah. This should just be "%t/libbar.dylib", testing "%t/:%t/libbar.dylib" is unnecessary... I had this right in an earlier version of the diff :p

int3 updated this revision to Diff 337033.Apr 12 2021, 8:13 PM
int3 marked an inline comment as done.

fix

gkm added inline comments.Apr 13 2021, 1:39 PM
lld/test/MachO/reroot-path.s
29–30

Almost there! These also need to drop the %:t/ component and become %t/libbar.dylib on the lld command line.

int3 added inline comments.Apr 13 2021, 2:04 PM
lld/test/MachO/reroot-path.s
29–30

It looks right to me as-is. Are you looking at the right version of the diff?

gkm added inline comments.Apr 13 2021, 5:08 PM
lld/test/MachO/reroot-path.s
29–30

Line 29 is good now with %t/libbar.dylib, but 28 still contains %t/%:t/libbar.dylib

int3 marked 2 inline comments as done.Apr 13 2021, 5:19 PM
int3 added inline comments.
lld/test/MachO/reroot-path.s
29–30

ugh, yes. Sorry, not sure how I missed that

int3 updated this revision to Diff 337291.Apr 13 2021, 5:19 PM
int3 marked an inline comment as done.

fix

gkm accepted this revision.Apr 13 2021, 10:06 PM

😄 😄 😄

This revision is now accepted and ready to land.Apr 13 2021, 10:06 PM
thakis added a subscriber: thakis.Apr 23 2021, 9:16 AM

Does this do the right thing with --reproduce? (cf createResponseFile() in DriverUtils.cpp)

int3 added a comment.Apr 23 2021, 9:07 PM

Does this do the right thing with --reproduce? (cf createResponseFile() in DriverUtils.cpp)

... kinda. It mostly works, except when there's another file at the non-rerooted absolute path, in which case we emit the wrong path. Fix coming up in a bit.