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

Fri Feb 26 13:01:08 PST 2021

oontvoo marked 3 inline comments as done.
oontvoo added a comment.

In D97438#2588902 <https://reviews.llvm.org/D97438#2588902>, @jyknight wrote:

> As for the test: Looks like you have only one TBD file, rather than the two in the original example: Cocoa (in Cocoa.tbd) re-exports AppKit (in AppKit.tbd) re-exports UIFoundation (also in AppKit.tbd). For your test, if you add a new "SystemReexport.tbd" which re-exports libSystem, I think that'd show the problem.

I've added a libReexportSystem.tbd, which reexport libSystem.tbd but still coulnd't get it to reproduce the re-export error ... :(  
I think we have to tell the test to say load the libReexportSystem right?

In D97438#2590738 <https://reviews.llvm.org/D97438#2590738>, @int3 wrote:

> Like the bundle_loader diff, this one needs more motivating detail in the commit message. It doesn't help that the Cocoa / AppKit libraries on my system don't include TBDs (perhaps because I'm not yet on Big Sur), so I'm not entirely sure what your commit message example is supposed to show. (And we should not assume that future readers will have a local system that matches yours, either.)
>
> @jyknight's comment helps clarify things a bit, but I'm still not sure why we need a parent pointer instead of the global `currentTopLevelTapi` pointer that we currently have. Is it because we might have a TBD file with child documents that re-exports another TBD file, also with child documents?

Yes, that's correct.  Say you have `A` re-exports `B`, `B` (which has some child docs) reexports `c` (where `c` is  defined in `B`)
If you keep the `globalTopLevelTapi` as `A`, then in `reexportHelper`, you can't find `c` in there.

================
Comment at: lld/test/MachO/Inputs/iPhoneSimulator.sdk/usr/lib/libSystem.tbd:7-9
+reexported-libraries:
+  - archs:      [i386, x86_64]
+    libraries:  ['/usr/lib/system/libsystem_sim_pthread.dylib']
----------------
int3 wrote:
> int3 wrote:
> > how is this different from the `re-exports` list on line 12 below?
> > 
> > (Scanning through TextStub.cpp, it seems like `reexported-libraries` is a TBD v4 thing, but this file is TBD v3, so I'm not sure it's doing anything. Are the system TBDs on your machine v4?)
> (perhaps this is why your test wasn't failing as expected)
Yes, good point. This is TBD v4.
I've reverted changes to this file and added a new libReexportSystem (as suggested by jyknight)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97438