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

[Jez Ng via Phabricator via llvm-commits]

int3 marked 6 inline comments as done.
int3 added inline comments.


================
Comment at: lld/MachO/ExportTrie.cpp:244
+
+  void parse(const uint8_t *buf, Twine cumulativeString);
+
----------------
smeenai wrote:
> smeenai wrote:
> > 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.)
> Oh, wait, `operator+` is a non-member function, so it doesn't need to be `const`. `const Twine &` should work here then, I think.
oh good point about TCO! Never realized it could cause issues like that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79226