[PATCH] D79226: [lld-macho] Use export trie instead of symtab when linking against dylibs

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai accepted this revision.
smeenai added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: lld/MachO/ExportTrie.cpp:244
+
+  void parse(const uint8_t *buf, Twine cumulativeString);
+
----------------
Can we make this a `Twine &` instead? Really, a `const Twine &` should have sufficed, but Twine.h doesn't mark the various `operator+` as const even though `concat` is const :/ (I'll look into fixing that.)

The reason I'm suggesting a reference is that a twine doesn't own any memory, so that if you e.g. concatenate a Twine and a StringRef, the result Twine will store a pointer to that StringRef. Theoretically, in `parse` below, the compiler could convert the recursive call in the last iteration in the loop to a tail call, since you're passing the Twine by value (and it's small enough to be passed in directly on at least certain ABIs), and both Twine and StringRef have trivial destructors. In that case, the StringRef pointer stored by that Twine will become dangling (since it's pointing to a stack variable in a frame that'll be reused for the tail call). Taking the argument by reference should prevent this by preventing any possibility of a tail call.

(In practice, I don't think the tail call optimization will happen, but eh. I'm also not sure if the x86-64 ABI would let Twines be passed by value in practice either, but it's best to not assume the details of any particular ABI.)


================
Comment at: lld/MachO/ExportTrie.cpp:250
+  const uint8_t *end;
+  const TrieEntryCallback &f;
+};
----------------
Nit: I'd name this `callback` or something else more descriptive than `f` :)


================
Comment at: lld/MachO/InputFiles.cpp:256
+  } else {
+    error("LC_DYLD_INFO_ONLY not found");
   }
----------------
You should include the input file name in this error message.


================
Comment at: lld/test/MachO/dylink.s:15
+# RUN: llvm-strip %t/libhello.dylib
+# RUN: llvm-strip %t/libgoodbye.dylib
+
----------------
Might be worth running an nm or similar after to confirm that the symbol table is empty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79226