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

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Driver.h:21
+class InterfaceFile;
+}
+} // namespace llvm
----------------



================
Comment at: lld/MachO/InputFiles.cpp:544
 // the first document storing child pointers to the rest of them. When we are
 // processing a given TBD file, we store that top-level document here. When
 // processing re-exports, we search its children for potentially matching
----------------
since this comment no longer precedes a variable declaration, it's not entirely clear what it's referring to any more :)


================
Comment at: lld/MachO/InputFiles.cpp:546-551
-// documents in the same TBD file. Note that the children themselves don't
-// point to further documents, i.e. this is a two-level tree.
-//
-// ld64 allows a TAPI re-export to reference documents nested within other TBD
-// files, but that seems like a strange design, so this is an intentional
-// deviation.
----------------
this comment still seems relevant 


================
Comment at: lld/MachO/InputFiles.cpp:706
 
-  bool isTopLevelTapi = false;
-  if (currentTopLevelTapi == nullptr) {
-    currentTopLevelTapi = &interface;
-    isTopLevelTapi = true;
-  }
+  const InterfaceFile *top_level = nullptr;
+  // If there is no top-level (meaning this tbd is the top-level) or if the
----------------
naming convention is camelCase


================
Comment at: lld/MachO/InputFiles.cpp:707-708
+  const InterfaceFile *top_level = nullptr;
+  // If there is no top-level (meaning this tbd is the top-level) or if the
+  // current interface has no parent
+  if (interface.getParent() == nullptr)
----------------
Isn't the current interface having no parent the same as a TAPI being the top-level one?


================
Comment at: lld/MachO/InputFiles.cpp:709-712
+  if (interface.getParent() == nullptr)
+    top_level = &interface;
+  else
+    top_level = interface.getParent(); // Second+ level exporters.
----------------
nit: this could be a ternary

also, since there are only two levels in the document tree, I guess this should just be "second level exporters", no "+"


================
Comment at: lld/test/MachO/Inputs/iPhoneSimulator.sdk/usr/lib/libReexportSystem.tbd:6
+platform:         ios
+install-name:     '/usr/lib/ReexportSystem.dylib'
+reexported-libraries:
----------------
usually the install name matches the tbd name


================
Comment at: lld/test/MachO/Inputs/iPhoneSimulator.sdk/usr/lib/libReexportSystem.tbd:8
+reexported-libraries:
+  - archs:      [i386, x86_64]
+    libraries:  ['/usr/lib/libSystem.B.dylib']
----------------



================
Comment at: lld/test/MachO/reexport-nested-lib.s:2
+# REQUIRES: x86
+# RUN: mkdir -p %t
+
----------------



================
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
+
----------------
I like to 


================
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:
> 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.


================
Comment at: lld/test/MachO/reexport-nested-lib.s:14
+.data
+  .quad __crashreporter_info__
----------------
I would add a comment explaining why `__crashreporter_info__` is an interesting symbol to test here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97438