This is an archive of the discontinued LLVM Phabricator instance.

[lld] [mach-o] Support -l and -syslibroot options
ClosedPublic

Authored by t.p.northover on Jul 7 2014, 10:15 AM.

Details

Reviewers
t.p.northover
Summary

Hi,

The attached patch implements the -l and -syslibroot options for the
Darwin lld driver. They're in the same patch because testing -l (looks
in /usr/lib and /usr/local/lib) without being able to override the
prefix is rather problematic.

I think the implementation itself is reasonably straightforward (the
one wrinkle is iterating through all arguments now, as opposed to just
OPT_INPUT and OPT_l. Not *strictly* necessary yet, but adding more
filter OPT_nnn arguments to the iterator doesn't scale and we do have
a lot more to support in the end).

The most controversial part is likely to be the testing: it adds
crafted binary MachO inputs, which isn't ideal if they ever need
updating (unlikely, they simply export a single, given symbol). This
isn't exactly unique though, so hopefully won't cause any problems.

OK to commit?

Cheers.

Tim

(Added to Phabricator at Shankar's request)

Diff Detail

Event Timeline

t.p.northover retitled this revision from to [lld] [mach-o] Support -l and -syslibroot options.
t.p.northover updated this object.
t.p.northover edited the test plan for this revision. (Show Details)
t.p.northover added a subscriber: Unknown Object (MLST).
shankarke added inline comments.
include/lld/ReaderWriter/MachOLinkingContext.h
134

should this be setSysRoot ?

lib/Driver/DarwinLdOptions.td
72

I dont see this an option in the ld64 man page, is this fairly new ?

http://www.manpages.info/macosx/ld.1.html

78

nit pick : searchf for -> search for.

lib/ReaderWriter/MachO/MachOLinkingContext.cpp
271
  • Can the the system paths be prepended to a set of directories that are being looked up, This could help adding for -L.
  • We can possibly search for a library name outside the loop.
300
    • The gnu linker has a way to have the sys root paths be appeneded to the path only if the path starts with an equal to(=) sign, Does MachO also do something like this ? If so this needs to be changed.
  • Does not account for -L

Hi Shankar,

Thanks for the comments. I'm not sure I have complete responses to any of them, but I'll have a go...

Cheers.

Tim.

include/lld/ReaderWriter/MachOLinkingContext.h
134

The option is -syslibroot on Darwin. I could see calling the field sysroot anyway, to maintain consistency with ELFLinkingContext. Does lld have any policy on where consistency is preferred?

lib/Driver/DarwinLdOptions.td
72

You'd have to ask someone like Nick for its actual age, but it certainly exists now.

lib/ReaderWriter/MachO/MachOLinkingContext.cpp
271

I started with that system, but specifically moved to this one for two reasons:

+ -syslibroot only applies to the system directories, so I'd need some way of making sure they stay special anyway.
+ Similarly there's a -Z option that removes just these from the search path. This could be handled by dealing with that before any -L or similar, of course.

On the whole, I think the static array is the neatest solution.

300

On the first: I saw that special handling, but I don't believe ld64 has it. The current ld64 man-pages only mention treating "y.o" specially.

On the second: I know. Incremental development and all that. I'd have separated out -syslibroot and -l if I could have reasonably tested one without the other.

Thanks for the detailed information, Tim. Could you wait if Nick has anything to say here ?

kledzik added a subscriber: kledzik.Jul 7 2014, 1:50 PM

This sort of testing won't scale up to all the combinations of searching the linker needs to do.

My suggestion is to add a hidden linker option* and change all uses of llvm::sys::fs::exists() to call a new MachOLinkingContext::fileExists(StringRef) method. Normally, fileExists() calls through to llvm::sys::fs::exists(), but if the new option is used, instead it prints out the path and returns false. Now you can write test cases with various combinations of options and then use FileCheck to verify all the correct files and checked for in the right order (for instance -r mode should not look for .dylib files).

  • There are various ways expose the option. It could be a command line arg, or an environment variable, or a DEBUG_WITH_TYPE() thing. Each has pros and cons. Also, since this option causes all libraries to fail to be found (so the full search is shown), we may want to have the option also trigger the linker to stop early (setDoNothing()) so the test cases don't have to deal with the errors from "missing" files.
include/lld/ReaderWriter/MachOLinkingContext.h
82–83

"path" is ambiguous is that it can refer to a file or directory. It would clearer to me to have this method as:

ErrorOr<StringRef> searchDirForLibrary(StringRef dir, StringRef libName) const

lib/ReaderWriter/MachO/MachOLinkingContext.cpp
271

Tim , -syslibroot applies to all -L paths - not just the built in ones, or the "system directories".

The algorithm is that if -syslibroot is used, to walk all search paths and if prepending that path with the syslibroot (aka SDK) path results in a directory, than switch that search path to use the SDK one.

You can add the -v option to ld64 and see it print out the final search paths.

275–276

You can avoid the copy-thru-std::string by using:

return fullPath.str().copy(_allocator);

Tim , -syslibroot applies to all -L paths - not just the built in ones, or the "system directories".

I don't think it does:

Ah, I see it applies to a basically randomly chosen set of paths. That
we'll almost certainly have to emulate. What fun! I'm rapidly
developing an abiding horror for linkers.

Tim.

Ah, I see it applies to a basically randomly chosen set of paths. That
we'll almost certainly have to emulate. What fun! I'm rapidly
developing an abiding horror for linkers.

That actual algorithm seems to be closer to:

  1. Add -L options to the search path (if it exists and is a directory).
  2. Record -syslibroot paths for later.
  3. Unless the last one is "/". Then discard the lot, because logic is for wimps.
  4. Try the cartesian product of syslibroots and *absolute* library

paths. Add ones that exist to the search path.

  1. If not, add the original unless there's just one syslibroot and

it's a default path. Then skip it.

  1. Do all the above in a particular order.

I'll fix the patch to do something similar, if I don't lose the will
to live first.

Cheers.

Tim.

I think this new version emulates the ld64 behaviour, certainly with much greater fidelity than before:

  • multiple -syslibroot options are accepted.
  • They apply to absolute paths (not visible yet because no -L).
  • Various quirks around single or special -syslibroot args.

I've also added a variant of the testing mode Nick suggested (in addition to the actual linking test), with a couple of modifications.

  • I decided that printing each queried path as it happened was too algorithm-dependent, when what we're really interested in is the outcome, so the actual behaviour is closer to ld64's "-v" option and prints out the final search paths.
  • I also decided that all pathExists calls failing was too inflexible and wouldn't allow us to actually test the algorithm's behaviour in most cases, so I also added a -path_exists override.

There's still no -L support, that'll be coming later (though it should be fairly simple). I think I've addressed the other comments though.

So how does this version look?

Cheers.

Tim.

t.p.northover added inline comments.Jul 9 2014, 5:14 AM
test/mach-o/libresolve-syslibroot.yaml
1 ↗(On Diff #11189)

Just noticed this file is a duplicate of libresolve-simple.yaml. I've removed it from my branch.

I'd change SmallString<128> to SmallString<256> because most sdk usages are buried inside Xcode.app and start at 120 chars.

Otherwise, LGTM

lib/Driver/DarwinLdOptions.td
72–73

A better help message would be "path to SDK"

78

HelpText<"Base name of library searched for in -L directories">;

t.p.northover accepted this revision.Jul 10 2014, 4:30 AM
t.p.northover added a reviewer: t.p.northover.

Thanks Nick. I've committed it with the changes you suggested in r212706. (Sorry about missing the typo again too. It *is* fixed by your new suggested wording in the commit).

Cheers.

Tim.

This revision is now accepted and ready to land.Jul 10 2014, 4:30 AM
t.p.northover closed this revision.Jul 10 2014, 4:30 AM