[lld-macho] De-templatize mach_header operations

[int3]

Summary

@thakis pointed out that mach_header and mach_header_64
actually have the same set of (used) fields, with the 64-bit version
having extra padding. So we can access the fields we need using the
single mach_header type instead of using templates to switch between
the two.

I also spotted a potential issue where hasObjCSection tries to parse a
file w/o checking if it does indeed match the target arch... As such,
I've added a quick magic number check to ensure we don't access invalid
memory during findCommand().

Addresses PR50180.

[int3]

remove a cast

[int3]

add magic number check

[int3]

lint

[thakis]

Nice, thanks!

lld/MachO/SyntheticSections.cpp

130

We should probably memset(buf + sizeof(mac_header), target->headerSize - sizeof(mac_header), 0) here to make sure that the reserved field is set to some deterministic data here. (Or is the output pre-zeroed? Probably not?)

[int3]

lld/MachO/SyntheticSections.cpp

130

There are several other places in the code that assume the output is pre-zeroed... and in practice that seems to work... though I'm not sure why 🤔

lld/MachO/Driver.cpp

for (const CachedHashStringRef &cachedName :
          config->exportedSymbols.literals) {
       if (const Symbol *sym = symtab->find(cachedName))
         if (isa<Defined>(sym))
           continue;
       error("undefined symbol " + cachedName.val() +
             "\n>>> referenced from option -exported_symbol(s_list)");
     }
 
-    if (target->wordSize == 8)
-      createSyntheticSections<LP64>();
-    else
-      createSyntheticSections<ILP32>();
-
+    createSyntheticSections();
     createSyntheticSymbols();
 
     for (const Arg *arg : args.filtered(OPT_sectcreate)) {
       StringRef segName = arg->getValue(0);
       StringRef sectName = arg->getValue(1);
       StringRef fileName = arg->getValue(2);
       Optional<MemoryBufferRef> buffer = readFile(fileName);
       if (buffer)

lld/MachO/InputFiles.h

 //===- InputFiles.h ---------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLD_MACHO_INPUT_FILES_H
 #define LLD_MACHO_INPUT_FILES_H
 
 #include "MachOStructs.h"
+#include "Target.h"
 
 #include "lld/Common/LLVM.h"
 #include "lld/Common/Memory.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/BinaryFormat/MachO.h"
 #include "llvm/DebugInfo/DWARF/DWARFUnit.h"
 #include "llvm/Object/Archive.h"
public:
   bool reexport = false;
   bool forceWeakImport = false;
 
   // An executable can be used as a bundle loader that will load the output
   // file being linked, and that contains symbols referenced, but not
   // implemented in the bundle. When used like this, it is very similar
   // to a Dylib, so we re-used the same class to represent it.
   bool isBundleLoader;
-
-private:
-  template <class LP> void parse(DylibFile *umbrella);
 };
 
 // .a file
 class ArchiveFile : public InputFile {
 public:
   explicit ArchiveFile(std::unique_ptr<llvm::object::Archive> &&file);
   static bool classof(const InputFile *f) { return f->kind() == ArchiveKind; }
   void fetch(const llvm::object::Archive::Symbol &sym);
public:
 
   std::unique_ptr<llvm::lto::InputFile> obj;
 };
 
 extern llvm::SetVector<InputFile *> inputFiles;
 
 llvm::Optional<MemoryBufferRef> readFile(StringRef path);
 
-template <class CommandType = llvm::MachO::load_command, class Header,
-          class... Types>
-const CommandType *findCommand(const Header *hdr, Types... types) {
+// anyHdr should be a pointer to either mach_header or mach_header_64
+template <class CommandType = llvm::MachO::load_command, class... Types>
+const CommandType *findCommand(const void *anyHdr, Types... types) {
   std::initializer_list<uint32_t> typesList{types...};
-  const uint8_t *p = reinterpret_cast<const uint8_t *>(hdr) + sizeof(Header);
-
+  auto *hdr = reinterpret_cast<const llvm::MachO::mach_header *>(anyHdr);
+  const uint8_t *p =
+      reinterpret_cast<const uint8_t *>(hdr) + target->headerSize;
   for (uint32_t i = 0, n = hdr->ncmds; i < n; ++i) {
     auto *cmd = reinterpret_cast<const CommandType *>(p);
     if (llvm::is_contained(typesList, cmd->cmd))
       return cmd;
     p += cmd->cmdsize;
   }
   return nullptr;
 }
 
 } // namespace macho
 
 std::string toString(const macho::InputFile *file);
 } // namespace lld
 
 #endif

lld/MachO/InputFiles.cpp

 
 static VersionTuple decodeVersion(uint32_t version) {
   unsigned major = version >> 16;
   unsigned minor = (version >> 8) & 0xffu;
   unsigned subMinor = version & 0xffu;
   return VersionTuple(major, minor, subMinor);
 }
 
-template <class LP>
 static Optional<PlatformInfo> getPlatformInfo(const InputFile *input) {
   if (!isa<ObjFile>(input) && !isa<DylibFile>(input))
     return None;
 
-  using Header = typename LP::mach_header;
-  auto *hdr = reinterpret_cast<const Header *>(input->mb.getBufferStart());
+  const char *hdr = input->mb.getBufferStart();
 
   PlatformInfo platformInfo;
   if (const auto *cmd =
           findCommand<build_version_command>(hdr, LC_BUILD_VERSION)) {
     platformInfo.target.Platform = static_cast<PlatformKind>(cmd->platform);
     platformInfo.minimum = decodeVersion(cmd->minos);
     return platformInfo;
   }
if (const auto *cmd = findCommand<version_min_command>(
     }
     platformInfo.minimum = decodeVersion(cmd->version);
     return platformInfo;
   }
 
   return None;
 }
 
-template <class LP> static bool checkCompatibility(const InputFile *input) {
-  Optional<PlatformInfo> platformInfo = getPlatformInfo<LP>(input);
+static bool checkCompatibility(const InputFile *input) {
+  Optional<PlatformInfo> platformInfo = getPlatformInfo(input);
   if (!platformInfo)
     return true;
   // TODO: Correctly detect simulator platforms or relax this check.
   if (config->platform() != platformInfo->target.Platform) {
     error(toString(input) + " has platform " +
           getPlatformName(platformInfo->target.Platform) +
           Twine(", which is different from target platform ") +
           getPlatformName(config->platform()));
template <class LP> void ObjFile::parse() {
   Architecture arch = getArchitectureFromCpuType(hdr->cputype, hdr->cpusubtype);
   if (arch != config->arch()) {
     error(toString(this) + " has architecture " + getArchitectureName(arch) +
           " which is incompatible with target architecture " +
           getArchitectureName(config->arch()));
     return;
   }
 
-  if (!checkCompatibility<LP>(this))
+  if (!checkCompatibility(this))
     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 (path.consume_front("/System/Library/Frameworks/")) {
     return path::filename(path) == frameworkName;
   }
 
   return false;
 }
 
 void loadReexport(StringRef path, DylibFile *umbrella,
                   const InterfaceFile *currentTopLevelTapi) {
   Optional<DylibFile *> reexport =
       findDylib(path, umbrella, currentTopLevelTapi);
   if (!reexport)
     error("unable to locate re-export with install name " + path);
   else if (isImplicitlyLinked(path))
     inputFiles.insert(*reexport);
 }
 
 DylibFile::DylibFile(MemoryBufferRef mb, DylibFile *umbrella,
                      bool isBundleLoader)
     : InputFile(DylibKind, mb), refState(RefState::Unreferenced),
       isBundleLoader(isBundleLoader) {
   assert(!isBundleLoader || !umbrella);
   if (umbrella == nullptr)
     umbrella = this;
 
-  if (target->wordSize == 8)
-    parse<LP64>(umbrella);
-  else
-    parse<ILP32>(umbrella);
-}
-
-template <class LP> void DylibFile::parse(DylibFile *umbrella) {
-  using Header = typename LP::mach_header;
   auto *buf = reinterpret_cast<const uint8_t *>(mb.getBufferStart());
-  auto *hdr = reinterpret_cast<const Header *>(mb.getBufferStart());
+  auto *hdr = reinterpret_cast<const mach_header *>(mb.getBufferStart());
 
   // Initialize dylibName.
   if (const load_command *cmd = findCommand(hdr, LC_ID_DYLIB)) {
     auto *c = reinterpret_cast<const dylib_command *>(cmd);
     currentVersion = read32le(&c->dylib.current_version);
     compatibilityVersion = read32le(&c->dylib.compatibility_version);
     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 (!checkCompatibility<LP>(this))
+  if (!checkCompatibility(this))
     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;
                 symbols.push_back(symtab->addDylib(
                     saver.save(name), exportingFile, isWeakDef, isTlv));
               });
   } else {
     error("LC_DYLD_INFO_ONLY not found in " + toString(this));
     return;
   }
 
-  const uint8_t *p = reinterpret_cast<const uint8_t *>(hdr) + sizeof(Header);
+  const uint8_t *p =
+      reinterpret_cast<const uint8_t *>(hdr) + target->headerSize;
   for (uint32_t i = 0, n = hdr->ncmds; i < n; ++i) {
     auto *cmd = reinterpret_cast<const load_command *>(p);
     p += cmd->cmdsize;
 
     if (!(hdr->flags & MH_NO_REEXPORTED_DYLIBS) &&
         cmd->cmd == LC_REEXPORT_DYLIB) {
       const auto *c = reinterpret_cast<const dylib_command *>(cmd);
       StringRef reexportPath =
BitcodeFile::BitcodeFile(MemoryBufferRef mbref)
   // Convert LTO Symbols to LLD Symbols in order to perform resolution. The
   // "winning" symbol will then be marked as Prevailing at LTO compilation
   // time.
   for (const lto::InputFile::Symbol &objSym : obj->symbols())
     symbols.push_back(createBitcodeSymbol(objSym, *this));
 }
 
 template void ObjFile::parse<LP64>();
-template void DylibFile::parse<LP64>(DylibFile *umbrella);

lld/MachO/ObjC.cpp

 using namespace lld;
 using namespace lld::macho;
 
 template <class LP> static bool hasObjCSection(MemoryBufferRef mb) {
   using Section = typename LP::section;
 
   auto *hdr =
       reinterpret_cast<const typename LP::mach_header *>(mb.getBufferStart());
+  if (hdr->magic != LP::magic)
+    return false;
+
   if (const auto *c =
           findCommand<typename LP::segment_command>(hdr, LP::segmentLCType)) {
     auto sectionHeaders =
         ArrayRef<Section>{reinterpret_cast<const Section *>(c + 1), c->nsects};
     for (const Section &sec : sectionHeaders) {
       StringRef sectname(sec.sectname,
                          strnlen(sec.sectname, sizeof(sec.sectname)));
       StringRef segname(sec.segname, strnlen(sec.segname, sizeof(sec.segname)));

lld/MachO/SyntheticSections.h

public:
   uint64_t getSize() const override final {
     return llvm::alignTo(getRawSize(), align);
   }
 };
 
 // The header of the Mach-O file, which must have a file offset of zero.
 class MachHeaderSection : public SyntheticSection {
 public:
-  void addLoadCommand(LoadCommand *);
+  MachHeaderSection();
   bool isHidden() const override { return true; }
+  uint64_t getSize() const override;
+  void writeTo(uint8_t *buf) const override;
+
+  void addLoadCommand(LoadCommand *);
 
 protected:
-  MachHeaderSection();
   std::vector<LoadCommand *> loadCommands;
   uint32_t sizeOfCmds = 0;
 };
 
-template <class LP> MachHeaderSection *makeMachHeaderSection();
-
 // A hidden section that exists solely for the purpose of creating the
 // __PAGEZERO segment, which is used to catch null pointer dereferences.
 class PageZeroSection : public SyntheticSection {
 public:
   PageZeroSection();
   bool isHidden() const override { return true; }
   uint64_t getSize() const override { return target->pageZeroSize; }
   uint64_t getFileSize() const override { return 0; }

lld/MachO/SyntheticSections.cpp

MachHeaderSection::MachHeaderSection()
   index = 1;
 }
 
 void MachHeaderSection::addLoadCommand(LoadCommand *lc) {
   loadCommands.push_back(lc);
   sizeOfCmds += lc->getSize();
 }
 
-// This serves to hide (type-erase) the template parameter from
-// MachHeaderSection.
-template <class LP> class MachHeaderSectionImpl : public MachHeaderSection {
-public:
-  MachHeaderSectionImpl() = default;
-  uint64_t getSize() const override;
-  void writeTo(uint8_t *buf) const override;
-};
-
-template <class LP> MachHeaderSection *macho::makeMachHeaderSection() {
-  return make<MachHeaderSectionImpl<LP>>();
-}
-
-template <class LP> uint64_t MachHeaderSectionImpl<LP>::getSize() const {
-  uint64_t size =
-      sizeof(typename LP::mach_header) + sizeOfCmds + config->headerPad;
+uint64_t MachHeaderSection::getSize() const {
+  uint64_t size = target->headerSize + sizeOfCmds + config->headerPad;
   // If we are emitting an encryptable binary, our load commands must have a
   // separate (non-encrypted) page to themselves.
   if (config->emitEncryptionInfo)
     size = alignTo(size, target->getPageSize());
   return size;
 }
 
 static uint32_t cpuSubtype() {
   uint32_t subtype = target->cpuSubtype;
 
   if (config->outputType == MH_EXECUTE && !config->staticLink &&
       target->cpuSubtype == CPU_SUBTYPE_X86_64_ALL &&
       config->platform() == PlatformKind::macOS &&
       config->platformInfo.minimum >= VersionTuple(10, 5))
     subtype |= CPU_SUBTYPE_LIB64;
 
   return subtype;
 }
 
-template <class LP>
-void MachHeaderSectionImpl<LP>::writeTo(uint8_t *buf) const {
-  auto *hdr = reinterpret_cast<typename LP::mach_header *>(buf);
-  hdr->magic = LP::magic;
+void MachHeaderSection::writeTo(uint8_t *buf) const {
+  auto *hdr = reinterpret_cast<mach_header *>(buf);
+  hdr->magic = target->magic;
   hdr->cputype = target->cpuType;
   hdr->cpusubtype = cpuSubtype();
   hdr->filetype = config->outputType;
   hdr->ncmds = loadCommands.size();
   hdr->sizeofcmds = sizeOfCmds;
   hdr->flags = MH_DYLDLINK;
 
   if (config->namespaceKind == NamespaceKind::twolevel)
void MachHeaderSection::writeTo(uint8_t *buf) const {
 
   for (const OutputSegment *seg : outputSegments) {
     for (const OutputSection *osec : seg->getSections()) {
       if (isThreadLocalVariables(osec->flags)) {
         hdr->flags |= MH_HAS_TLV_DESCRIPTORS;
         break;
       }
     }
   }
 
-  uint8_t *p = reinterpret_cast<uint8_t *>(hdr + 1);
+  uint8_t *p = reinterpret_cast<uint8_t *>(hdr) + target->headerSize;
   for (const LoadCommand *lc : loadCommands) {
     lc->writeTo(p);
     p += lc->getSize();
   }
 }
 
 PageZeroSection::PageZeroSection()
     : SyntheticSection(segment_names::pageZero, section_names::pageZero) {}
void macho::createSyntheticSymbols() {
   // The Itanium C++ ABI requires dylibs to pass a pointer to __cxa_atexit
   // which does e.g. cleanup of static global variables. The ABI document
   // says that the pointer can point to any address in one of the dylib's
   // segments, but in practice ld64 seems to set it to point to the header,
   // so that's what's implemented here.
   addHeaderSymbol("___dso_handle");
 }
 
-template MachHeaderSection *macho::makeMachHeaderSection<LP64>();
-template MachHeaderSection *macho::makeMachHeaderSection<ILP32>();
 template SymtabSection *macho::makeSymtabSection<LP64>(StringTableSection &);
 template SymtabSection *macho::makeSymtabSection<ILP32>(StringTableSection &);

lld/MachO/Target.h

 class DylibSymbol;
 class InputSection;
 
 class TargetInfo {
 public:
   template <class LP> TargetInfo(LP) {
     // Having these values available in TargetInfo allows us to access them
     // without having to resort to templates.
+    magic = LP::magic;
     pageZeroSize = LP::pageZeroSize;
+    headerSize = sizeof(typename LP::mach_header);
     wordSize = LP::wordSize;
   }
 
   virtual ~TargetInfo() = default;
 
   virtual int64_t getEmbeddedAddend(const char *loc, uint32_t relType,
                                     uint32_t relLength) const = 0;
   virtual void relocateOne(uint8_t *loc, const Reloc &, uint64_t va,
public:
   virtual const RelocAttrs &getRelocAttrs(uint8_t type) const = 0;
 
   virtual uint64_t getPageSize() const = 0;
 
   bool hasAttr(uint8_t type, RelocAttrBits bit) const {
     return getRelocAttrs(type).hasAttr(bit);
   }
 
+  uint32_t magic;
   uint32_t cpuType;
   uint32_t cpuSubtype;
 
   uint64_t pageZeroSize;
+  size_t headerSize;
   size_t stubSize;
   size_t stubHelperHeaderSize;
   size_t stubHelperEntrySize;
   size_t wordSize;
 };
 
 TargetInfo *createX86_64TargetInfo();
 TargetInfo *createARM64TargetInfo();

lld/MachO/Writer.h

 public:
   virtual ~LoadCommand() = default;
   virtual uint32_t getSize() const = 0;
   virtual void writeTo(uint8_t *buf) const = 0;
 };
 
 template <class LP> void writeResult();
 
-template <class LP> void createSyntheticSections();
+void createSyntheticSections();
 
 // Add bindings for symbols that need weak or non-lazy bindings.
 void addNonLazyBindingEntries(const Symbol *, const InputSection *,
                               uint64_t offset, int64_t addend = 0);
 
 extern OutputSection *firstTLVDataSection;
 
 } // namespace macho
 } // namespace lld
 
 #endif

lld/MachO/Writer.cpp

template <class LP> void Writer::run() {
   finalizeAddresses();
   finalizeLinkEditSegment();
   writeMapFile();
   writeOutputFile();
 }
 
 template <class LP> void macho::writeResult() { Writer().run<LP>(); }
 
-template <class LP> void macho::createSyntheticSections() {
-  in.header = makeMachHeaderSection<LP>();
+void macho::createSyntheticSections() {
+  in.header = make<MachHeaderSection>();
   in.rebase = make<RebaseSection>();
   in.binding = make<BindingSection>();
   in.weakBinding = make<WeakBindingSection>();
   in.lazyBinding = make<LazyBindingSection>();
   in.exports = make<ExportSection>();
   in.got = make<GotSection>();
   in.tlvPointers = make<TlvPointerSection>();
   in.lazyPointers = make<LazyPointerSection>();
   in.stubs = make<StubsSection>();
   in.stubHelper = make<StubHelperSection>();
   in.imageLoaderCache = make<ImageLoaderCacheSection>();
   in.unwindInfo = makeUnwindInfoSection();
 }
 
 OutputSection *macho::firstTLVDataSection = nullptr;
 
 template void macho::writeResult<LP64>();
 template void macho::writeResult<ILP32>();
-template void macho::createSyntheticSections<LP64>();
-template void macho::createSyntheticSections<ILP32>();

lld/test/MachO/objc.s

 # REQUIRES: x86
 # RUN: rm -rf %t; split-file %s %t
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/has-objc-symbol.s -o %t/has-objc-symbol.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/has-objc-category.s -o %t/has-objc-category.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/has-swift.s -o %t/has-swift.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/no-objc.s -o %t/no-objc.o
-# RUN: llvm-ar rcs %t/libHasSomeObjC.a %t/has-objc-symbol.o %t/has-objc-category.o %t/has-swift.o %t/no-objc.o
+## Make sure we don't mis-parse a 32-bit file as 64-bit
+# RUN: llvm-mc -filetype=obj -triple=armv7-apple-watchos %t/no-objc.s -o %t/wrong-arch.o
+# RUN: llvm-ar rcs %t/libHasSomeObjC.a %t/has-objc-symbol.o %t/has-objc-category.o %t/has-swift.o %t/no-objc.o %t/wrong-arch.o
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/test.s -o %t/test.o
-# RUN: %lld -lSystem %t/test.o -o %t/test \
-# RUN:   -L%t -lHasSomeObjC -ObjC
+# RUN: %lld -lSystem %t/test.o -o %t/test -L%t -lHasSomeObjC -ObjC
 # RUN: llvm-objdump --section-headers --syms %t/test | FileCheck %s --check-prefix=OBJC
 
 # OBJC:       Sections:
 # OBJC-NEXT:  Idx Name           Size   VMA  Type
 # OBJC-NEXT:    0 __text         {{.*}}      TEXT
 # OBJC-NEXT:    1 __swift        {{.*}}      DATA
 # OBJC-NEXT:    2 __objc_catlist {{.*}}      DATA
 # OBJC-EMPTY:
 # OBJC-NEXT:  SYMBOL TABLE:
 # OBJC-NEXT:  g     F __TEXT,__text _main
 # OBJC-NEXT:  g     F __TEXT,__text _OBJC_CLASS_$_MyObject
 
-# RUN: %lld -lSystem %t/test.o -o %t/test \
-# RUN:   -L%t -lHasSomeObjC
+# RUN: %lld -lSystem %t/test.o -o %t/test -L%t -lHasSomeObjC
 # RUN: llvm-objdump --section-headers --syms %t/test | FileCheck %s --check-prefix=NO-OBJC
 
 # NO-OBJC:       Sections:
 # NO-OBJC-NEXT:  Idx Name           Size   VMA  Type
 # NO-OBJC-NEXT:    0 __text         {{.*}}      TEXT
 # NO-OBJC-EMPTY:
 # NO-OBJC-NEXT:  SYMBOL TABLE:
 # NO-OBJC-NEXT:  g     F __TEXT,__text _main

 ## This archive member should not be pulled in since it does not contain any
 ## ObjC-related data.
 .globl _foo
 .section __DATA,foo
 
 foo:
   .quad 0x1234
 
+.section __DATA,bar
+.section __DATA,baz
+
 #--- test.s
 .globl _main
 _main:
   ret