[lld-macho] Check platform and version while constructing ObjFile and DylibFile

[int3]

Neat. Could you clean up the clang-tidy issues? Kind of hard to review the code with all that in the way.

Also, I think the method is getting large enough, it would be nice to factor out the arch/platform check into a separate method.

Finally, don't forget about the test :)

[oontvoo]

Updated diff:

• made clang-tidy happy
• added test

[oontvoo]

more test

[int3]

lld/MachO/InputFiles.cpp

351

ultra nit: given that you're casting cmd->platform to a PlatformKind below, why not do it here too for uniformity?

364–368

how about constructing another VersionTuple instance and compare that against platformInfo.minimum?

that way you can also use VersionTuple::getAsString() below

513

no need to repeat the type name on the same line

677

seems kind of janky to pass in a function here just so we can prepend "dylib". I think it's clear enough that it's a dylib from the name (and if it's not, we should update toString so that all our error messages are uniform). Basically I think it would be cleaner to just pass in the InputFile *

lld/MachO/InputFiles.h

179

nice, I like the convenient casting. But I don't think there's a need to move this function into the header

[int3]

lld/test/MachO/incompat-version.s

1 ↗

(On Diff #328387)

Can we merge this test into invalid/incompatible-arch.s? (and rename the file to incompatible-target or something). That test file is pretty small as-is and this test is covering very similar functionality.

(In general we put tests that only check for error handling into invalid/.)

[oontvoo]

updated diff:

• moved test into existing incompatible-arch.s
• added additional test case to cover the ObjFile path
• fixed other styling issues

lld/MachO/InputFiles.h

179

It's a templated function, so you can't put it in the cpp file.

If we put it in the cpp file, then it can only be a static local function.

[int3]

lld/MachO/InputFiles.h

179

Oh, I didn't realize this was being used in ObjC.cpp

lld/test/MachO/invalid/incompatible-arch.s

11–12

doesn't seem like the -o %t/new.dylib is necessary given that we have -o /dev/null later

16

nit: use hyphens for uniformity with things like CHECK-NEXT

19–22

I'm guessing you created a bitcode file in order to specify the platform? You can actually specify it by compiling a regular assembly file via llvm-mc:

llvm-mc -filetype=obj -triple=x86_64-apple-macos14.0

[oontvoo]

Updated test to specify the platform version with llvm-mc rather than going through the trouble of using a bitcode file

lld/test/MachO/invalid/incompatible-arch.s

19–22

Good to know!

[int3]

lgtm!

lld/test/MachO/invalid/incompatible-arch.s

0–6

[oontvoo]

updated diff

[oontvoo]

rebase

[int3]

lld/MachO/InputFiles.cpp

345

oh I just noticed that you passed in the Configuration as a parameter. It's not necessary since config is a global singleton

[oontvoo]

lld/MachO/InputFiles.cpp

345

it was kind of intentional since I didn't like using a global variable in this context .
But now it seemed a lot of other places did that too. Sent a NFC to fix it at https://reviews.llvm.org/D98115

lld/MachO/InputFiles.h

public:
 
   std::unique_ptr<llvm::lto::InputFile> obj;
 };
 
 extern llvm::SetVector<InputFile *> inputFiles;
 
 llvm::Optional<MemoryBufferRef> readFile(StringRef path);
 
-const llvm::MachO::load_command *
-findCommand(const llvm::MachO::mach_header_64 *, uint32_t type);
+template <class CommandType = llvm::MachO::load_command>
+const CommandType *findCommand(const llvm::MachO::mach_header_64 *hdr,
+                               uint32_t type) {
+  const uint8_t *p = reinterpret_cast<const uint8_t *>(hdr) +
+                     sizeof(llvm::MachO::mach_header_64);
+
+  for (uint32_t i = 0, n = hdr->ncmds; i < n; ++i) {
+    auto *cmd = reinterpret_cast<const CommandType *>(p);
+    if (cmd->cmd == type)
+      return cmd;
+    p += cmd->cmdsize;
+  }
+  return nullptr;
+}
 
 } // namespace macho
 
 std::string toString(const macho::InputFile *file);
 } // namespace lld
 
 #endif

lld/MachO/InputFiles.cpp

if (tar)
       tar->append(relativeToRoot(path), mbref.getBuffer());
     return MemoryBufferRef(StringRef(buf + offset, size), path.copy(bAlloc));
   }
 
   error("unable to find matching architecture in " + path);
   return None;
 }
 
-const load_command *macho::findCommand(const mach_header_64 *hdr,
-                                       uint32_t type) {
-  const uint8_t *p =
-      reinterpret_cast<const uint8_t *>(hdr) + sizeof(mach_header_64);
-
-  for (uint32_t i = 0, n = hdr->ncmds; i < n; ++i) {
-    auto *cmd = reinterpret_cast<const load_command *>(p);
-    if (cmd->cmd == type)
-      return cmd;
-    p += cmd->cmdsize;
-  }
-  return nullptr;
-}
-
 void ObjFile::parseSections(ArrayRef<section_64> sections) {
   subsections.reserve(sections.size());
   auto *buf = reinterpret_cast<const uint8_t *>(mb.getBufferStart());
 
   for (const section_64 &sec : sections) {
     InputSection *isec = make<InputSection>();
     isec->file = this;
     isec->name =
if (sym.n_type & (N_EXT | N_PEXT)) {
     assert((sym.n_type & N_EXT) && "invalid input");
     return symtab->addDefined(name, isec->file, isec, value,
                               sym.n_desc & N_WEAK_DEF, sym.n_type & N_PEXT);
   }
   return make<Defined>(name, isec->file, isec, value, sym.n_desc & N_WEAK_DEF,
                        /*isExternal=*/false, /*isPrivateExtern=*/false);
 }
 
+// Checks if the version specified in `cmd` is compatible with target
+// version in `config`. IOW, check if cmd's version >= config's version.
+static bool hasCompatVersion(const InputFile *input,
+                             const build_version_command *cmd,
+                             const Configuration *config) {
+
+  if (config->target.Platform != static_cast<PlatformKind>(cmd->platform)) {
+    error(toString(input) + " has platform " +
+          getPlatformName(static_cast<PlatformKind>(cmd->platform)) +
+          Twine(", which is different from target platform ") +
+          getPlatformName(config->target.Platform));
+    return false;
+  }
+
+  unsigned major = cmd->minos >> 16;
+  unsigned minor = (cmd->minos >> 8) & 0xffu;
+  unsigned subMinor = cmd->minos & 0xffu;
+  VersionTuple version(major, minor, subMinor);
+  if (version >= config->platformInfo.minimum)
+    return true;
+
+  error(toString(input) + " has version " + version.getAsString() +
+        ", which is incompatible with target version of " +
+        config->platformInfo.minimum.getAsString());
+  return false;
+}
+
 // Absolute symbols are defined symbols that do not have an associated
 // InputSection. They cannot be weak.
 static macho::Symbol *createAbsolute(const structs::nlist_64 &sym,
                                      InputFile *file, StringRef name) {
   if (sym.n_type & (N_EXT | N_PEXT)) {
     assert((sym.n_type & N_EXT) && "invalid input");
     return symtab->addDefined(name, file, nullptr, sym.n_value,
                               /*isWeakDef=*/false, sym.n_type & N_PEXT);
   }
ObjFile::ObjFile(MemoryBufferRef mb, uint32_t modTime, StringRef archiveName)
   MachO::Architecture arch =
       MachO::getArchitectureFromCpuType(hdr->cputype, hdr->cpusubtype);
   if (arch != config->target.Arch) {
     error(toString(this) + " has architecture " + getArchitectureName(arch) +
           " which is incompatible with target architecture " +
           getArchitectureName(config->target.Arch));
     return;
   }
-  // TODO: check platform too
+
+  if (const auto *cmd =
+          findCommand<build_version_command>(hdr, LC_BUILD_VERSION)) {
+    if (!hasCompatVersion(this, cmd, config))
+      return;
+  }
 
   if (const load_command *cmd = findCommand(hdr, LC_LINKER_OPTION)) {
     auto *c = reinterpret_cast<const linker_option_command *>(cmd);
     StringRef data{reinterpret_cast<const char *>(c + 1),
                    c->cmdsize - sizeof(linker_option_command)};
     parseLCLinkerOption(this, c->count, data);
   }
 
if (const load_command *cmd = findCommand(hdr, LC_ID_DYLIB)) {
     dylibName = reinterpret_cast<const char *>(cmd) + read32le(&c->dylib.name);
   } else if (!isBundleLoader) {
     // macho_executable and macho_bundle don't have LC_ID_DYLIB,
     // so it's OK.
     error("dylib " + toString(this) + " missing LC_ID_DYLIB load command");
     return;
   }
 
+  if (const build_version_command *cmd =
+          findCommand<build_version_command>(hdr, LC_BUILD_VERSION)) {
+    if (!hasCompatVersion(this, cmd, config))
+      return;
+  }
+
   // Initialize symbols.
   DylibFile *exportingFile = isImplicitlyLinked(dylibName) ? this : umbrella;
   if (const load_command *cmd = findCommand(hdr, LC_DYLD_INFO_ONLY)) {
     auto *c = reinterpret_cast<const dyld_info_command *>(cmd);
     parseTrie(buf + c->export_off, c->export_size,
               [&](const Twine &name, uint64_t flags) {
                 bool isWeakDef = flags & EXPORT_SYMBOL_FLAGS_WEAK_DEFINITION;
                 bool isTlv = flags & EXPORT_SYMBOL_FLAGS_KIND_THREAD_LOCAL;

lld/test/MachO/invalid/incompatible-arch.s

-# REQUIRES: aarch64
-# RUN: llvm-mc -filetype=obj -triple=arm64-apple-darwin %s -o %t.o
-# RUN: not %lld -arch x86_64 -lSystem %t.o -o /dev/null 2>&1 | FileCheck %s -DFILE=%t.o
+# REQUIRES: aarch64, x86
+
+# RUN: rm -rf %t && mkdir -p %t
+
+# RUN: llvm-mc -filetype=obj -triple=arm64-apple-darwin %s -o %t/test.o
+# RUN: not %lld -arch x86_64 -lSystem %t/test.o -o /dev/null 2>&1 | FileCheck %s -DFILE=%t/test.o
 # CHECK: error: {{.*}}[[FILE]] has architecture arm64 which is incompatible with target architecture x86_64
 
+# RUN: %lld -dylib  -arch arm64 -platform_version macOS 9.0 11.0 -o %t/out.dylib %t/test.o
+
+# RUN: not %lld -dylib -arch arm64 -platform_version iOS 9.0 11.0  %t/out.dylib  \
+# RUN:  -o /dev/null 2>&1 | FileCheck %s --check-prefix=DYLIB-PLAT
+# DYLIB-PLAT: {{.*}}out.dylib has platform macOS, which is different from target platform iOS
+
+# RUN: not %lld -dylib -arch arm64 -platform_version macOS 14.0 15.0  %t/out.dylib  \
+# RUN:  -o /dev/null 2>&1 | FileCheck %s --check-prefix=DYLIB-VERSION
+# DYLIB-VERSION: {{.*}}out.dylib has version 9.0.0, which is incompatible with target version of 14.0
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos10.15.0 %s -o %t/test_x86.o
+
+# RUN: not %lld %t/test_x86.o -lSystem -arch x86_64 -platform_version iOS 10.0 15.0 \
+# RUN:  -o /dev/null 2>&1 | FileCheck %s --check-prefix=OBJ-PLAT
+# OBJ-PLAT: {{.*}}test_x86.o has platform macOS, which is different from target platform iOS
+
+# RUN: not %lld %t/test_x86.o -lSystem -arch x86_64 -platform_version macOS 14.0 15.0 \
+# RUN:  -o /dev/null 2>&1 | FileCheck %s --check-prefix=OBJ-VERSION
+# OBJ-VERSION: {{.*}}test_x86.o has version 10.15.0, which is incompatible with target version of 14.0
+
 .globl _main
 _main:
   ret