[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 added inline comments.


================
Comment at: lld/test/MachO/reexport-nested-lib.s:5
+# RUN: llvm-mc -filetype obj -triple x86_64-apple-darwin %s -o %t/test.o
+# RUN: %lld -o %t/test  -syslibroot %S/Inputs/iPhoneSimulator.sdk -lSystem %t/test.o
+
----------------
int3 wrote:
> oontvoo wrote:
> > int3 wrote:
> > > int3 wrote:
> > > > int3 wrote:
> > > > > I like to 
> > > > lt doesn't seem like libReexportSystem is actually being used in this test
> > > > 
> > > > Also, there doesn't seem to be a need to use the iPhoneSimulator SDK over the macOS SDK here -- `%lld` defaults to include the macOS SDK as syslibroot, so it's a bit more convenient to use that.
> > > > 
> > > > Another nit: I would rather libReexportSystem to not be placed under `/usr/lib` -- but I acknowledge there's no strong rationale here, I just like our fake `/usr/lib` to look as much like a realistic `/usr/lib` as possible. Personally I would place that tbd directly under `%S/Inputs`.
> > > > 
> > > > Finally, I think it's nice to test that the binary contains the right symbol binding via `llvm-objdump --macho --bind`, rather than just assuming it from a successful LLD exit code.
> > > > I like to 
> > > 
> > > sorry, stray comment, ignore it
> > > lt doesn't seem like libReexportSystem is actually being used in this test
> > > 
> > > Also, there doesn't seem to be a need to use the iPhoneSimulator SDK over the macOS SDK here -- `%lld` defaults to include the macOS SDK as syslibroot, so it's a bit more convenient to use that.
> > > 
> > > Another nit: I would rather libReexportSystem to not be placed under `/usr/lib` -- but I acknowledge there's no strong rationale here, I just like our fake `/usr/lib` to look as much like a realistic `/usr/lib` as possible. Personally I would place that tbd directly under `%S/Inputs`.
> > > 
> > > Finally, I think it's nice to test that the binary contains the right symbol binding via `llvm-objdump --macho --bind`, rather than just assuming it from a successful LLD exit code.
> > 
> > 
> Did you forget to type something? You just quoted my comment in its entirety :P
> Did you forget to type something? You just quoted my comment in its entirety :P

Yes, sorry! I thought I typed the responses in-line. not sure why it's not  showing up.

Anyway, re-posting it again:

> lt doesn't seem like libReexportSystem is actually being used in this test

Fixed (updated the test to reference symbols from both the exporter and the exportee.

> Also, there doesn't seem to be a need to use the iPhoneSimulator SDK over the macOS SDK here -- %lld defaults to include the macOS SDK as syslibroot, so it's a bit more convenient to use that.

The macOS SDK's libSystem.tbd doesn't have a "benign" symbol (like crash_reporter symbol)  I could use, so I thought it was easier to use the iOS. 

> Another nit: I would rather libReexportSystem to not be placed under /usr/lib -- but I acknowledge there's no strong rationale here, I just like our fake /usr/lib to look as much like a realistic /usr/lib as possible. Personally I would place that tbd directly under %S/Inputs.
> Finally, I think it's nice to test that the binary contains the right symbol binding via llvm-objdump --macho --bind, rather than just 
assuming it from a successful LLD exit code.

Added the objdump CHECKs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97438