[PATCH] D97438: [lld-macho] Change loadReexport to handle the case where a TAPI re-exports to reference documents nested within other TBD.

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo marked an inline comment as done.
oontvoo added inline comments.


================
Comment at: lld/test/MachO/reexport-nested-lib.s:19-21
+// This symbol is from libSystem, which is re-exported by libReexportSystem.
+// Reference it here to verify that it is visible.
+.quad __crashreporter_info__
----------------
int3 wrote:
> I don't think this is testing the right thing. (The test passes for me even without your code changes applied.)
> 
> I believe the case we are trying to fix is as follows:
> 
> * libFooBar.tbd contains libFoo.dylib and libBar.dylib. libFoo re-exports libBar. libBar contains some symbol `Bar`.
> * libReexportsFoo.tbd contains libReexportsFoo.dylib which re-exports libFoo. It doesn't matter whether it re-exports anything else.
> 
> Prior to this fix, we would have failed to look up libBar because we would be looking inside libReexportsFoo.tbd, instead of libFoo.tbd.
> 
> Right now we are testing that a symbol in libFoo can be resolved by the linker, but we actually need to test for a symbol that's in a library that libFoo (libSystem in your case) re-exports. And FWIW, the fake macOS libSystem has `__nan` which should fit the bill :)
> 
> P.S. it would be good to have a comment that explains the motivation behind the test. Right now the comment talks about re-exports but doesn't touch on the TBD files that contain them, which I think is the key bit.
Yeah, that's the right summary of it. :)  Thanks.

Updated the test. Confirmed that w/o the patch, it'd fail with something like this:

```
--
lld: error: unable to locate re-export with install name /usr/lib/system/libdyld.dylib
lld: error: unable to locate re-export with install name /usr/lib/system/libsystem_c.dylib
lld: error: unable to locate re-export with install name /usr/lib/system/libsystem_m.dylib
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97438