This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Add support for creating and reading reexported dylibs
ClosedPublic

Authored by int3 on Apr 30 2020, 9:35 PM.

Details

Summary

Mach-O dylibs can re-export other dylibs as sub-libraries, meaning that the
symbols in those sub-libraries will be available under the umbrella
library's namespace.

Supporting this unblocks the linking of real programs, since many core
system functions are only available as sub-libraries of libSystem.

Depends on D79211.

Diff Detail

Event Timeline

int3 created this revision.Apr 30 2020, 9:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2020, 9:35 PM
int3 edited the summary of this revision. (Show Details)May 1 2020, 12:53 AM
smeenai added inline comments.May 8 2020, 1:54 PM
lld/MachO/Driver.cpp
121

This seems pretty inefficient in a bunch of ways in that we're iterating over the entire input file list (only a small number of which will be dylibs) multiple times (once for each -sub_library option). I don't know how bad it'll end up being in practice though, and there's a bunch of straightforward ways to make it better if need be, so I guess it's okay for now.

179
lld/MachO/InputFiles.cpp
131

Do you envision this being used in other places? If not, I'd just inline the loop in the one place it's being used right now; I know @ruiu hasn't been a fan of higher-order functions in the past.

294

Is it an error if a library has this flag set but has no re-exports? (I don't think it's terribly important to implement; just curious.)

306

Hmm. This isn't correct in that @executable_path is supposed to refer to the path of the exectuable that'll be loading this library (which we have no way of knowing right now). @loader_path would be the path of the library that's triggering the load.

Where are you seeing @executable_path be used for re-exports? Hmm, is it from the install name of sub-libraries getting copied over to the re-export load commands? Not sure what the best way to handle that is ... I'd be curious what ld64 does. One idea that springs to mind is to see if the current dylib's install name also begins with @executable_path/, and if so, compute the path of the other dylib relative to the current dylib. (As in, if your current dylib's install name is @executable_path/libfoo.dylib and the one you're re-exporting is @executable_path/../lib/libbar.dylib, it follows that the re-exported dylib should live in ../lib relative to the current dylib. On the other hand, if you flipped that around, the current dylib would have no way of knowing which subdirectory of its parent the re-exported dylib is supposed to live in...)

lld/MachO/Options.td
26

What about -reexport-lx and -reexport_library foo? Those seem to be the more modern options.

int3 marked 6 inline comments as done.May 9 2020, 10:19 AM
int3 added inline comments.
lld/MachO/Driver.cpp
121

yup, this is definitely quadratic, but I figured input file lists + sub_library options would probably be relatively small

lld/MachO/InputFiles.cpp
131

No plans to use it in other places yet; can inline for now

294

yup! That's why I was always setting it prior to this diff. IIRC dyld would complain

306

Ah, yeah I misunderstood what @executable_path was supposed to represent...

is it from the install name of sub-libraries getting copied over to the re-export load commands?

yup

I'd be curious what ld64 does

I think it loads sub-libraries after parsing the entire command line. I guess our equivalent would be putting the executable path in the Config object, though I'm a bit wary of bloating Config...

Might be easier to just have the test use absolute paths for now and punt on implementing this till later

lld/MachO/Options.td
26

I just wanted to implement enough sub-library creation functionality to test our ability to read them. I think we can punt full support for sub-library creation till later

int3 updated this revision to Diff 263025.May 9 2020, 10:19 AM

address comments

smeenai accepted this revision.May 11 2020, 11:54 PM

LGTM with the unused function removed.

lld/MachO/InputFiles.cpp
305

Same here.

lld/MachO/InputFiles.h
85

This is unused now, right?

lld/MachO/Options.td
26

Sounds good.

This revision is now accepted and ready to land.May 11 2020, 11:54 PM
int3 marked 3 inline comments as done.May 12 2020, 7:50 AM
int3 added inline comments.
lld/MachO/InputFiles.h
85

whoops, good catch

int3 updated this revision to Diff 263438.May 12 2020, 7:51 AM
int3 marked an inline comment as done.

address comments

This revision was automatically updated to reflect the committed changes.