[PATCH] D130725: [lld/mac] Add support for $ld$previous symbols with explicit symbol name

[Jez Ng via Phabricator via llvm-commits]

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

just some questions about the comments but lgtm



================
Comment at: lld/MachO/InputFiles.cpp:2077
+
+    // Just adding the symbol to the symtab works because dylibs contain their
+    // symbols in alphabetical order, guaranteeing $ld$ symbols to precede
----------------
what do you mean by "works" here? what's the failure case that we have avoided?


================
Comment at: lld/MachO/InputFiles.h:244
+
   bool explicitlyLinked = false;
+  bool isExplicitlyLinked() const;
----------------
can we make this private so it's clear that we should be calling `isExplicitlyLinked()`? Looks like it's only being written to from `loadDylib` but we could make that a friend function if you find a setter too ugly


================
Comment at: lld/MachO/InputFiles.h:245
   bool explicitlyLinked = false;
+  bool isExplicitlyLinked() const;
+
----------------
nit: would prefer if all the methods were located together (so at line 227 above) instead of interleaved with fields


================
Comment at: lld/test/MachO/special-symbol-ld-previous.s:33-34
+## Case 4: special symbol $ld$previous affects the install name / compatibility version
+## when the specified version 11.0.0 is within the affected range [3.0, 14.0) when a symbol
+## is part of $previous$ if and only if that symbol is referenced.
+
----------------
> when a symbol is part of $previous$ if and only if that symbol is referenced.

kind of confused by this. Did you mean "only when the named symbol is referenced"?


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

https://reviews.llvm.org/D130725