Page MenuHomePhabricator

MachO: support `-syslibroot`
ClosedPublic

Authored by compnerd on Jun 19 2020, 9:16 PM.

Details

Reviewers
smeenai
Ktwu
int3
Group Reviewers
Restricted Project
Commits
rG0ccda7c2326e: MachO: support `-syslibroot`
Summary

This adds support for the -syslibroot option. This is required to
make the library search order actually function. With this, it is now
possible to link a test Darwin x86_64 program with lld on Darwin.

Diff Detail

Event Timeline

compnerd created this revision.Jun 19 2020, 9:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2020, 9:17 PM
int3 added inline comments.Jun 24 2020, 12:31 PM
lld/MachO/Driver.cpp
162

any particular reason why you picked 261?

174

suppresses

lld/test/MachO/syslibroot.test
2

s/non-existant/nonexistent/

4

s/NONEXISTANT/NONEXISTENT/

15

nit: could we have a shorter tmp directory name?

16

nit: most other tests use double-dash flags, i.e. --check-prefix

19

nit: would be better to pass the tmp dir to FileCheck via -DTMP=%t and use it here instead of {{.+}}. I believe the current check would pass even if there were only whitespace before /Users/...

int3 requested changes to this revision.Jul 2 2020, 4:51 PM

clearing my queue

This revision now requires changes to proceed.Jul 2 2020, 4:51 PM
compnerd marked an inline comment as done.Fri, Jul 24, 11:23 AM
compnerd added inline comments.
lld/MachO/Driver.cpp
162

MAX(256, 261) - 261 is the Windows path limit, 256 is the value used on Unix.

compnerd marked 5 inline comments as done.Sat, Aug 1, 9:37 AM
compnerd added inline comments.
lld/test/MachO/syslibroot.test
15

Sure, we can make it a system library path instead. IMO, the longer path immediately identifies that this is definitely not a default search path (/System/Library/Frameworks vs /Library/Frameworks is slightly easier to miss).

compnerd updated this revision to Diff 282401.Sat, Aug 1, 9:37 AM
compnerd marked an inline comment as done.

Address review feedback

compnerd planned changes to this revision.Sat, Aug 1, 9:41 AM
compnerd requested review of this revision.
int3 added a comment.Sat, Aug 1, 1:53 PM

Oh yeah, don't forget to remove the HelpHidden tag from syslibroot's definition in Options.td

lld/MachO/Driver.cpp
160

Will this work on Windows given the use of Style::posix?

162

Ah I see. Can we make it a named constant?

lld/test/MachO/syslibroot.test
22–23

which is the fuzzy match here? not sure I follow...

24

is /var/empty also going to be empty on Windows? might be better to create our own empty tmp directory instead

int3 added inline comments.Mon, Aug 3, 8:57 AM
lld/MachO/Driver.cpp
161

codebase convention is to avoid auto unless the type is immediately obvious

164

tested it out locally and realized that these isDirectory checks are now going to spew dir-not-found errors... we should probably factor out the error logging

424

avoid auto

int3 added a comment.Mon, Aug 3, 11:29 AM

search-paths.test needs to be updated too

compnerd added inline comments.Mon, Aug 3, 5:41 PM
lld/MachO/Driver.cpp
160

Yes, it should work on windows just as well. I've been running the tests on Windows :)

161

I think its obvious given that roots is just declared above, but Im fine with replacing it with StringRef.

162

Id rather do that in a follow up. There are other places where we have file path sized buffers.

164

Hmm, would you prefer to use llvm::sys::fs::is_directory instead?

424

const auto arg is pretty common, but sure, Arg *arg is shorter.

lld/test/MachO/syslibroot.test
22–23

/var/empty is not being checked.

24

No, /var/empty will not exist on Windows, but the fuzzy match takes care of that. The intent of the /var/empty is ensure that a non-sysroot is not treated as a sysroot.

compnerd marked 3 inline comments as done.Tue, Aug 4, 9:24 AM
compnerd updated this revision to Diff 282950.Tue, Aug 4, 9:41 AM
int3 accepted this revision.Tue, Aug 4, 10:04 AM

lgtm modulo those two questions. Thanks!

lld/MachO/Driver.cpp
160

I guess my question then is how does it work? I was under the impression that we would want the style to be native since the path separators are different on Windows.

(I'm working on setting up a Windows VM so I can answer these things myself in the future 😅)

162

sgtm

164

seems better, thanks!

lld/test/MachO/syslibroot.test
22–23

ah I see. Maybe make that explicit in the comment?

Also, shouldn't we be able to portably check that /var/empty isn't present? My understanding is that the -syslibroot / will make -syslibroot /var/empty a no-op on all platforms... is that right?

This revision is now accepted and ready to land.Tue, Aug 4, 10:04 AM
compnerd added inline comments.Tue, Aug 4, 1:28 PM
lld/MachO/Driver.cpp
160

Windows paths are dark and full of terrors. The Win32 APIs will re-normalize the paths and accept / and \ (though not the NT APIs). I opted for the posix representation to make the printing work out better. We can always change this in the future.

lld/test/MachO/syslibroot.test
22–23

Sure, I can make that more explicit in the comment.

The problem is that -syslibroot / will then pick up /usr/lib and /usr/local/lib which makes the test more brittle. This was basically my attempt to force the test to be more uniform across Linux/BSD/Darwin/Windows.

This revision was automatically updated to reflect the committed changes.