[Object] Extend MachOUniversalBinary::getObjectForArch

[alexander-shaposhnikov]

Summary

A particular slice of a universal binary can be either a MachO object or an archive
(although the class MachOUniversalBinary doesn't store them explicitly, instead, they can be constructed "on demand"
via calling the method ObjectForArch::getAsObjectFile or ObjectForArch::getAsArchive)
Before this patch MachOUniversalBinary had only a method (getObjectForArch)
to look up a slice for a particular architecture assuming that the slice
is a MachO object. We generalize it and make it return the corresponding instance of ObjectForArch instead;

Test plan: make check-all

[compnerd]

include/llvm/Object/MachOUniversal.h

162 ↗

(On Diff #220639)

ObjectForArch? That seems like an odd type. Why not use Binary if you are looking for a common base type for an object file and an archive?

[mtrent]

include/llvm/Object/MachOUniversal.h

162 ↗

(On Diff #220639)

IIUC, the idea is to restrict the kind of things that one might reasonably find in a universal file. For example, constrain the output from getObjectForArch to be either a Mach-O file or an Archive. Binary would imply you would have to deal with the possibility that a universal file might contain another universal file, or a non-binary file such as a plist, ELF binaries, or literally anything.

Because people can make universal files themselves (it's a very simple file format), we find sometimes people abuse the format and shove weird stuff in there. Sometimes it's something "wrong-but-benign" like shoving an macOS-compiled simulator framework alongside an iOS-compiled framework. Sometimes it's something "wrong-just-wrong".

ObjectForArch is an existing class meant to put the burden of checking types on MachOUniversal.cpp. Returning Binary would put the burden of checking types onto each caller. I agree its a weird type name to expose to callers.

A reasonable alternative would be to add "getArchiveForArch" alongside "getObjectForArch" and then provide a third "isObject / isArchive" accessor so callers knew which method to call. That's probably the same order of patch as this change.

[alexander-shaposhnikov]

include/llvm/Object/MachOUniversal.h

162 ↗

(On Diff #220639)

@compnerd
Just wanted to add (I also mentioned this in the description), that I was looking at
the implementation of MachOUniversalBinary::ObjectForArch::getAsObjectFile / MachOUniversalBinary::ObjectForArch::getAsArchive. The thing is that the both methods create
MachOObjectFile / Archive instances on "the fly" using ObjectFile::createMachOObjectFile / Archive::create respectively. The both methods can return Error for multiple reasons.
Unpacking Expected<MachOObjectFile> and Expected<Archive> and picking the "right" error didn't look good to me, that's why preferred the current approach.

[compnerd]

Okay, both @mtrent and @alexshap bring up good points. I *suppose* that is okay, but I shall string grumble. @mtrent, I think we could also simply make getObjectForArch be internal to the type, and force users to go down either getMachOArchiveForArch or getMachOObjectForArch.

llvm/include/llvm/Object/MachOUniversal.h

public:
   uint32_t getMagic() const { return Magic; }
   uint32_t getNumberOfObjects() const { return NumberOfObjects; }
 
   // Cast methods.
   static bool classof(Binary const *V) {
     return V->isMachOUniversalBinary();
   }
 
-  Expected<std::unique_ptr<MachOObjectFile>>
+  Expected<ObjectForArch>
   getObjectForArch(StringRef ArchName) const;
+
+  Expected<std::unique_ptr<MachOObjectFile>>
+  getMachOObjectForArch(StringRef ArchName) const;
+
+  Expected<std::unique_ptr<Archive>>
+  getArchiveForArch(StringRef ArchName) const;
 };
 
 }
 }
 
 #endif

llvm/lib/DebugInfo/Symbolize/Symbolize.cpp

if (!Bin)
     return static_cast<ObjectFile *>(nullptr);
 
   if (MachOUniversalBinary *UB = dyn_cast_or_null<MachOUniversalBinary>(Bin)) {
     auto I = ObjectForUBPathAndArch.find(std::make_pair(Path, ArchName));
     if (I != ObjectForUBPathAndArch.end())
       return I->second.get();
 
     Expected<std::unique_ptr<ObjectFile>> ObjOrErr =
-        UB->getObjectForArch(ArchName);
+        UB->getMachOObjectForArch(ArchName);
     if (!ObjOrErr) {
       ObjectForUBPathAndArch.emplace(std::make_pair(Path, ArchName),
                                      std::unique_ptr<ObjectFile>());
       return ObjOrErr.takeError();
     }
     ObjectFile *Res = ObjOrErr->get();
     ObjectForUBPathAndArch.emplace(std::make_pair(Path, ArchName),
                                    std::move(ObjOrErr.get()));

llvm/lib/Object/MachOUniversal.cpp

for (uint32_t j = i + 1; j < NumberOfObjects; j++) {
           + Twine(B.getSize()));
         return;
       }
     }
   }
   Err = Error::success();
 }
 
-Expected<std::unique_ptr<MachOObjectFile>>
+Expected<MachOUniversalBinary::ObjectForArch>
 MachOUniversalBinary::getObjectForArch(StringRef ArchName) const {
   if (Triple(ArchName).getArch() == Triple::ArchType::UnknownArch)
     return make_error<GenericBinaryError>("Unknown architecture "
                                           "named: " +
                                               ArchName,
                                           object_error::arch_not_found);
-
-  for (auto &Obj : objects())
+  for (const auto &Obj : objects())
     if (Obj.getArchFlagName() == ArchName)
-      return Obj.getAsObjectFile();
+      return Obj;
   return make_error<GenericBinaryError>("fat file does not "
                                         "contain " +
                                             ArchName,
                                         object_error::arch_not_found);
 }
+
+Expected<std::unique_ptr<MachOObjectFile>>
+MachOUniversalBinary::getMachOObjectForArch(StringRef ArchName) const {
+  Expected<ObjectForArch> O = getObjectForArch(ArchName);
+  if (!O)
+    return O.takeError();
+  return O->getAsObjectFile();
+}
+
+Expected<std::unique_ptr<Archive>>
+MachOUniversalBinary::getArchiveForArch(StringRef ArchName) const {
+  Expected<ObjectForArch> O = getObjectForArch(ArchName);
+  if (!O)
+    return O.takeError();
+  return O->getAsArchive();
+}

llvm/lib/Object/Object.cpp

 }
 
 LLVMBinaryRef LLVMMachOUniversalBinaryCopyObjectForArch(LLVMBinaryRef BR,
                                                         const char *Arch,
                                                         size_t ArchLen,
                                                         char **ErrorMessage) {
   auto universal = cast<MachOUniversalBinary>(unwrap(BR));
   Expected<std::unique_ptr<ObjectFile>> ObjOrErr(
-      universal->getObjectForArch({Arch, ArchLen}));
+      universal->getMachOObjectForArch({Arch, ArchLen}));
   if (!ObjOrErr) {
     *ErrorMessage = strdup(toString(ObjOrErr.takeError()).c_str());
     return nullptr;
   }
   return wrap(ObjOrErr.get().release());
 }
 
 LLVMSectionIteratorRef LLVMObjectFileCopySectionIterator(LLVMBinaryRef BR) {

llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp

 }
 
 static Expected<std::unique_ptr<BinaryCoverageReader>>
 loadBinaryFormat(std::unique_ptr<Binary> Bin, StringRef Arch) {
   std::unique_ptr<ObjectFile> OF;
   if (auto *Universal = dyn_cast<MachOUniversalBinary>(Bin.get())) {
     // If we have a universal binary, try to look up the object for the
     // appropriate architecture.
-    auto ObjectFileOrErr = Universal->getObjectForArch(Arch);
+    auto ObjectFileOrErr = Universal->getMachOObjectForArch(Arch);
     if (!ObjectFileOrErr)
       return ObjectFileOrErr.takeError();
     OF = std::move(ObjectFileOrErr.get());
   } else if (isa<ObjectFile>(Bin.get())) {
     // For any other object file, upcast and take ownership.
     OF.reset(cast<ObjectFile>(Bin.release()));
     // If we've asked for a particular arch, make sure they match.
     if (!Arch.empty() && OF->getArch() != Triple(Arch).getArch())

llvm/tools/llvm-lipo/llvm-lipo.cpp

static void verifyArch(ArrayRef<OwningBinary<Binary>> InputBinaries,
   assert(InputBinaries.size() == 1 && "Incorrect number of input binaries");
 
   for (StringRef Arch : VerifyArchList)
     validateArchitectureName(Arch);
 
   if (auto UO =
           dyn_cast<MachOUniversalBinary>(InputBinaries.front().getBinary())) {
     for (StringRef Arch : VerifyArchList) {
-      Expected<std::unique_ptr<MachOObjectFile>> Obj =
-          UO->getObjectForArch(Arch);
-      if (!Obj)
+      auto ObjForArch = UO->getObjectForArch(Arch);
+      if (!ObjForArch)
         exit(EXIT_FAILURE);
     }
   } else if (auto O =
                  dyn_cast<MachOObjectFile>(InputBinaries.front().getBinary())) {
     const Triple::ArchType ObjectArch = O->getArch();
     for (StringRef Arch : VerifyArchList)
       if (ObjectArch != Triple(Arch).getArch())
         exit(EXIT_FAILURE);
if (InputBinaries.front().getBinary()->isMachO()) {
     reportError("input file " +
                 InputBinaries.front().getBinary()->getFileName() +
                 " must be a fat file when the -thin option is specified");
     exit(EXIT_FAILURE);
   }
 
   auto *UO = cast<MachOUniversalBinary>(InputBinaries.front().getBinary());
   Expected<std::unique_ptr<MachOObjectFile>> Obj =
-      UO->getObjectForArch(ThinArchType);
+      UO->getMachOObjectForArch(ThinArchType);
   if (!Obj)
     reportError("fat input file " + UO->getFileName() +
                 " does not contain the specified architecture " + ThinArchType +
                 " to thin it to");
 
   Expected<std::unique_ptr<FileOutputBuffer>> OutFileOrError =
       FileOutputBuffer::create(OutputFileName,
                                Obj.get()->getMemoryBufferRef().getBufferSize(),

llvm/tools/llvm-objdump/MachODump.cpp

if (!DSYMFile.empty()) {
         } else {
           const MachO::mach_header H = MachOOF->getHeader();
           CPUType = H.cputype;
           CPUSubType = H.cpusubtype;
         }
         Triple T = MachOObjectFile::getArchTriple(CPUType, CPUSubType, nullptr,
                                                   &ArchFlag);
         Expected<std::unique_ptr<MachOObjectFile>> MachDSYM =
-            UB->getObjectForArch(ArchFlag);
+            UB->getMachOObjectForArch(ArchFlag);
         if (!MachDSYM) {
           reportError(MachDSYM.takeError(), DSYMFile);
           return;
         }
     
         // We need to keep the Binary elive with the buffer
         DbgObj = &*MachDSYM.get();
         DSYMBinary = std::move(*MachDSYM);