[PATCH] D103505: [lld][MachO] Add first bits to support special symbols

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Config.h:41
 
+constexpr uint32_t encodeVersion(const llvm::VersionTuple &version) {
+  return ((version.getMajor() << 020) |
----------------
why `constexpr`?


================
Comment at: lld/MachO/InputFiles.cpp:955
 
+bool DylibFile::handleLdSymbol(StringRef name) {
+  // $ld$ previous $ <installname> $ <compatversion> $ <platformstr> $
----------------
this needs a comment explaining what $ld$ symbols are

(would be good to have that in the commit message too)


================
Comment at: lld/MachO/InputFiles.cpp:963-964
+  std::tie(action, name) = name.drop_front(4 /* $ld$ */).split('$');
+  if (action.empty() || action != "previous")
+    return false;
+
----------------
Perhaps we should return `true` here, since these are clearly not meant to be interpreted as regular symbols. (Same for the other `return false` lines below.)

Also maybe leave a comment / FIXME about the other actions?


================
Comment at: lld/MachO/InputFiles.cpp:988-989
+
+  llvm::VersionTuple start;
+  llvm::VersionTuple end;
+  if (start.tryParse(startVersion) || end.tryParse(endVersion) ||
----------------
no need for llvm::


================
Comment at: lld/MachO/InputFiles.cpp:990
+  llvm::VersionTuple end;
+  if (start.tryParse(startVersion) || end.tryParse(endVersion) ||
+      config->platformInfo.minimum < start ||
----------------
`tryParse` returning `true` for a failed parse is a truly bizarre API...

perhaps we should log a warning when that happens?


================
Comment at: lld/MachO/InputFiles.cpp:999-1000
+    llvm::VersionTuple cVersion;
+    if (cVersion.tryParse(compatVersion))
+      return false;
+    compatibilityVersion = encodeVersion(cVersion);
----------------
ditto, I think we should log a warning here (and test for it)


================
Comment at: lld/test/MachO/Inputs/libLDPreviousInstallName.tbd:1-11
+--- !tapi-tbd-v3
+archs:           [ x86_64 ]
+uuids:           [ 'x86_64: 19311019-01AB-342E-812B-73A74271A715' ]
+platform:        macosx
+install-name:    '/System/Library/LDPreviousInstallNameOld'
+current-version: 5
+compatibility-version: 1.1.1
----------------
I learned from @thakis today that you can use `split-file` together with tbds if you pass `--no-leading-lines` :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103505