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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: lld/MachO/Driver.cpp:132
 
-    if (Optional<std::string> path = findWithExtension(symlink, {".tbd", ""}))
+    if (Optional<std::string> path = resolveDylibPath(symlink))
       return path;
----------------
int3 wrote:
> smeenai wrote:
> > IIUC, the previous version would prefer the tbd to the dylib, and `resolveDylibPath` will prefer the dylib to the TBD.
> > 
> > IIRC, if ld64 finds both a TBD and a dylib for the same file, it verifies the TBD against the dylib and falls back to the dylib in case of a mismatch. I don't think we need to implement that (and I don't think LLVM's version of TextAPI has the equivalence checking logic yet anyway), so perhaps always preferring the dylib in that case (as you're doing now) is reasonable.
> yeah, I did inadvertently change things here. From a quick glance at the code, I think your description of ld64's behavior is correct. ld64 can also take an environment variable that makes it always prefer the TAPI file. Do you know if having both the TAPI and the dylib has a real use case? Given the amount of logic ld64 has devoted to it, I guess there might be, but it seems odd. Maybe I could just raise an error for now if both are present?
The Xcode 12 betas have both dylibs and TBDs present (for at least some of the SDKs), so erroring out in that case wouldn't be ideal. (I don't think the final Xcode releases every have dylibs.) I think it's fine to just use the dylib if you have both, and perhaps add a TODO to consider adding verification like ld64 has.


================
Comment at: lld/MachO/InputFiles.cpp:375
+
+  for (InterfaceFile &intf : make_pointee_range(inlineDocuments))
+    if (path == intf.getInstallName())
----------------
int3 wrote:
> smeenai wrote:
> > If I'm understanding correctly, this will only be able to find any inline documents we've seen in our input files thus far. Does this match ld64, or is it able to find inline documents from later files?
> ld64 is indeed able to find documents from later files. It seems like pretty bizarre behavior to me, though -- what would be the use case? (In my initial implementation, I actually tried to limit re-exports in tbds to reference just those in the same file because I thought that made the most sense, but it was more awkward to implement that...)
Yeah, I'd also find it weird to reference sub-documents from other files, and I don't know if it's actually used in the SDKs.


================
Comment at: lld/test/MachO/reexport-stub.s:16
+# CHECK: Bind table:
+# CHECK-DAG: __DATA __data {{.*}} pointer 0 libreexporter ___gxx_personality_v0
+
----------------
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
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85404