[PATCH] D85404: [lld-macho] Handle TAPI and regular re-exports uniformly

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai accepted this revision.
smeenai added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks for limiting re-exports to sub-documents in the same TBD!



================
Comment at: lld/MachO/InputFiles.cpp:370
+// deviation.
+const InterfaceFile *currentTopLevelTapi = nullptr;
+
----------------
Nit: make this `static`.


================
Comment at: lld/test/MachO/reexport-stub.s:16
+# CHECK: Bind table:
+# CHECK-DAG: __DATA __data {{.*}} pointer 0 libreexporter ___gxx_personality_v0
+
----------------
int3 wrote:
> int3 wrote:
> > smeenai wrote:
> > > int3 wrote:
> > > > smeenai wrote:
> > > > > int3 wrote:
> > > > > > smeenai wrote:
> > > > > > > I thought that for sub-libraries, the bind information reflected the actual sub-library the symbol was coming from, and that seems to be the case in a basic test I did with ld64. Can you confirm how this is supposed to behave?
> > > > > > > 
> > > > > > > (IIRC there's options to re-export specific symbols, and those bind to the re-exporting library.)
> > > > > > Swapping ld64 for lld in this particular test gives the same result. (sub-library.s tests something similar, and ld64 binds to the parent library there too.) Can you share the test you did that showed a binding to the sub-library?
> > > > > I'm using ld64 530 from Xcode 11.3.1. If I try to run this test as-is (albeit with clang instead of llvm-mc), when I try to link `test`, I get an error:
> > > > > 
> > > > >   ld: malformed mach-o, symbol table not in __LINKEDIT file './libreexporter.dylib'
> > > > > 
> > > > > I can just add a dummy symbol to work around that, and then
> > > > > 
> > > > > ```
> > > > > $ cat reexporter.s
> > > > > .globl foo
> > > > > foo:
> > > > > 	ret
> > > > > 
> > > > > $ cat test.s
> > > > > .globl _main
> > > > > _main:
> > > > > 	ret
> > > > > .data
> > > > > 	.quad	___gxx_personality_v0
> > > > > 
> > > > > $ clang -c reexporter.s test.s
> > > > > $ ld -dylib -lc++ -sub_library libc++ reexporter.o -o libreexporter.dylib
> > > > > $ ld -o test -lSystem -L. -lreexporter test.o
> > > > > $ objdump --bind test
> > > > > Bind table:
> > > > > segment  section            address    type       addend dylib            symbol
> > > > > __DATA   __data             0x100001000 pointer         0 libc++           ___gxx_personality_v0
> > > > > ```
> > > > hm, I must have messed up my first attempt, because I was able to see the binding to libc++ on my second try. But I figured out why sub-library.s doesn't repro the issue: bindings only get routed directly to re-exports if those re-exports have "public install names", i.e. if they live under `/usr/lib/` or if they are a top-level framework under `/System/Library/Frameworks/`. I couldn't find any code comments explaining why, but I'm guessing it's an optimization -- I suppose the libraries with "public install names" form a stable ABI, and the only reason they get re-exported by other libraries is to provide some convenience to the user (who doesn't need to pass in half a dozen linker flags). Avoiding unnecessary re-export processing at runtime is probably a performance win.
> > > > 
> > > > Addressing this seems somewhat orthogonal to this diff, so a TODO comment should suffice for now?
> > > Ah, interesting. And yup, a TODO sounds good.
> > I've added the TODO to the `DylibFile` ctor.
> The manpage entry for `-no_implicit_dylibs` confirms my guess:
> 
> ```
>      -no_implicit_dylibs
>                  When creating a two-level namespace final linked image, normally the
>                  linker will hoist up public dylibs that are implicitly linked to make the
>                  two-level namespace encoding more efficient for dyld.  For example, Cocoa
>                  re-exports AppKit and AppKit re-exports Foundation.  If you link with
>                  -framework Cocoa and use a symbol from Foundation, the linker will
>                  implicitly add a load command to load Foundation and encode the symbol as
>                  coming from Foundation.  If you use this option, the linker will not add
>                  a load command for Foundation and encode the symbol as coming from Cocoa.
>                  Then at runtime dyld will have to search Cocoa and AppKit before finding
>                  the symbol in Foundation.
> ```
Ah, good find.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85404/new/

https://reviews.llvm.org/D85404