Page MenuHomePhabricator

[lld-macho] Use export trie instead of symtab when linking against dylibs
ClosedPublic

Authored by int3 on Thu, Apr 30, 8:02 PM.

Details

Summary

This allows us to link against stripped dylibs. Moreover, it's simply
more correct: The symbol table includes symbols that the dylib uses but
doesn't export.

This temporarily regresses our ability to do lazy symbol binding because
dyld_stub_binder isn't in libSystem's export trie. Rather, it is in one
of the sub-libraries libSystem re-exports. (This doesn't affect our
tests since we are mocking out dyld_stub_binder there.) A follow-up diff
will address this by adding support for sub-libraries.

Depends on D79114.

Diff Detail

Event Timeline

int3 created this revision.Thu, Apr 30, 8:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Apr 30, 8:02 PM
smeenai accepted this revision.Wed, May 6, 1:29 AM

LGTM

lld/MachO/ExportTrie.cpp
244

Can we make this a Twine & instead? Really, a const Twine & should have sufficed, but Twine.h doesn't mark the various operator+ as const even though concat is const :/ (I'll look into fixing that.)

The reason I'm suggesting a reference is that a twine doesn't own any memory, so that if you e.g. concatenate a Twine and a StringRef, the result Twine will store a pointer to that StringRef. Theoretically, in parse below, the compiler could convert the recursive call in the last iteration in the loop to a tail call, since you're passing the Twine by value (and it's small enough to be passed in directly on at least certain ABIs), and both Twine and StringRef have trivial destructors. In that case, the StringRef pointer stored by that Twine will become dangling (since it's pointing to a stack variable in a frame that'll be reused for the tail call). Taking the argument by reference should prevent this by preventing any possibility of a tail call.

(In practice, I don't think the tail call optimization will happen, but eh. I'm also not sure if the x86-64 ABI would let Twines be passed by value in practice either, but it's best to not assume the details of any particular ABI.)

250

Nit: I'd name this callback or something else more descriptive than f :)

lld/MachO/InputFiles.cpp
256

You should include the input file name in this error message.

lld/test/MachO/dylink.s
15

Might be worth running an nm or similar after to confirm that the symbol table is empty.

This revision is now accepted and ready to land.Wed, May 6, 1:29 AM
smeenai added inline comments.Wed, May 6, 5:19 PM
lld/MachO/ExportTrie.cpp
244

Oh, wait, operator+ is a non-member function, so it doesn't need to be const. const Twine & should work here then, I think.

int3 marked 6 inline comments as done.Wed, May 6, 8:04 PM
int3 added inline comments.
lld/MachO/ExportTrie.cpp
244

oh good point about TCO! Never realized it could cause issues like that.

int3 updated this revision to Diff 262532.Wed, May 6, 8:04 PM
int3 marked an inline comment as done.

address comments

MaskRay accepted this revision.Fri, May 8, 7:47 AM
MaskRay added inline comments.
lld/MachO/ExportTrie.h
40

clang-format may prefer const llvm::Twine & /*name*/

int3 updated this revision to Diff 262939.Fri, May 8, 1:14 PM

format

int3 marked an inline comment as done.Fri, May 8, 1:14 PM
smeenai accepted this revision.Fri, May 8, 1:30 PM

LGTM

This revision was automatically updated to reflect the committed changes.