[PATCH] D100523: [lld][MachO] Add support for LC_VERSION_MIN_* load commands

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lld/MachO/Config.h:104
   bool demangle = false;
-  llvm::MachO::Target target;
   PlatformInfo platformInfo;
----------------
I intentionally left this in `Config` instead of `PlatformInfo` because I didn't have to have a whole bunch of member accesses to get to something commonly used...

But maybe I should just make accessor methods so we can do things like `config->arch()` and `config->target()` instead

(I can do it after this lands)

In the future please split out these sorts of refactoring changes into a separate diff


================
Comment at: lld/MachO/InputFiles.cpp:98
 
+static llvm::VersionTuple decodeVersion(uint32_t version) {
+  unsigned major = version >> 16;
----------------
no need for llvm::

(please %s/llvm:://g for the other changes in this file too)


================
Comment at: lld/MachO/InputFiles.cpp:106-107
+template <class LP>
+llvm::Optional<PlatformInfo> InputFile::getPlatformInfo() const {
+  if (!isa<ObjFile>(this) && !isa<DylibFile>(this))
+    return llvm::None;
----------------
how about having this be a file-static function (like `hasCompatVersion` was)?

same for `checkCompatibility`.


================
Comment at: lld/MachO/InputFiles.cpp:118-137
+  } else if (const auto *cmd =
+                 findCommand<version_min_command>(hdr, LC_VERSION_MIN_MACOSX)) {
+    platformInfo.target.Platform = PlatformKind::macOS;
+    platformInfo.minimum = decodeVersion(cmd->version);
+    return platformInfo;
+  } else if (const auto *cmd = findCommand<version_min_command>(
+                 hdr, LC_VERSION_MIN_IPHONEOS)) {
----------------
this could be simplified if we modified `findCommand` to look for multiple `LC_*` commands at once...

(I can do it in a follow up)


================
Comment at: lld/MachO/InputFiles.h:93-98
   const Kind fileKind;
   const StringRef name;
 
   static int idCount;
+
+  template <class LP> llvm::Optional<PlatformInfo> getPlatformInfo() const;
----------------
nit: methods above fields


================
Comment at: lld/MachO/Writer.cpp:362
 
+static uint32_t encodeVersion(const llvm::VersionTuple &version) {
+  return ((version.getMajor() << 020) |
----------------
%s/llvm:://g in this file too


================
Comment at: lld/MachO/Writer.cpp:369
+class LCMinVersion : public LoadCommand {
+  const PlatformInfo &platformInfo;
+
----------------
convention is to put private members at the bottom of the class declaration




================
Comment at: lld/MachO/Writer.cpp:395
+      break;
+    default:
+      llvm_unreachable("invalid platform");
----------------
I think clang should be able to see that this is exhaustive


================
Comment at: lld/MachO/Writer.cpp:406-407
 class LCBuildVersion : public LoadCommand {
+  const PlatformInfo &platformInfo;
+
 public:
----------------
convention is to put private members at the bottom of the class declaration


================
Comment at: lld/MachO/Writer.cpp:594
 
+// TODO: ld64 enforces the old load commands in a few other cases.
+static bool useLCBuildVersion(const PlatformInfo &platformInfo) {
----------------
would be good to have a comment explaining the rationale, i.e. that older platforms will look for the older LC_MIN_VERSION_* load command


================
Comment at: lld/MachO/Writer.cpp:596
+static bool useLCBuildVersion(const PlatformInfo &platformInfo) {
+  static const std::map<PlatformKind, llvm::VersionTuple> minVersion = {
+      {PlatformKind::macOS, llvm::VersionTuple(10, 14)},
----------------
nit: I would use a DenseMap here just to be consistent with the rest of LLVM


================
Comment at: lld/test/MachO/lc-build-version.s:30
+
+# IOS_12_0:      cmd LC_BUILD_VERSION
+
----------------



================
Comment at: lld/test/MachO/lc-build-version.s:44
+
+# TVOS_12_0:      cmd LC_BUILD_VERSION
+
----------------
we could define one `BUILD_VERSION: cmd LC_BUILD_VERSION` check prefix and use it multiple times... though if you think this is clearer that's fine too


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100523