[PATCH] D88400: [llvm-objcopy][MachO] Add support for universal binaries

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a comment.

LGTM barring the comment about more tests.



================
Comment at: llvm/lib/Object/MachOUniversalWriter.cpp:78
 
+Slice::Slice(const Archive &A, uint32_t CPUType, uint32_t CPUSubType,
+             std::string ArchName, uint32_t Align)
----------------
Is the expectation that this constructor will be used when the `Archive &` is from an existing universal binary, and it's therefore okay to bypass all the checks in the `Slice::create` overload that takes an `Archive &`? If so, we should explicitly comment that (preferably as a Doxygen comment in the header), otherwise people might be confused in the future about when they want to use which function.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/universal-object.test:1
+# This test verifies that llvm-objcopy copies a univeral Mach-O object file properly.
+
----------------
All of these tests are just for copies. We should also have some tests for performing actual operations (e.g. stripping) to show that we correctly apply the operation to each slice.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/universal-object.test:16
+## Case 2: copy a universal object file containing an archive.
+# RUN: llvm-ar cr %t.archive.i386 %t.i386
+# RUN: llvm-lipo %t.archive.i386 %t.x86_64 -create -output %t.universal.containing.archive
----------------
You probably want an `rm -f %t.archive.i386` before this, otherwise I believe ar will append to any existing file with that name (e.g. a leftover from a previous test run).


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/universal-object.test:25
+
+## Case 3: copy an archive containing a universal object.
+# RUN: llvm-ar cr %t.archive.containing.universal %t.universal
----------------
This is actually invalid, but I guess it doesn't hurt to support it. From the Apple man page for libtool:

> Ranlib takes all correct forms of libraries (universal files containing archives, and simple archives) and updates the table of contents for all archives in the file. Ranlib also takes one common incorrect form of archive, an archive whose members are universal object files, adding or updating the table of contents and producing the library in correct form (a universal file containing multiple archives).


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:396-397
+                                           Buffer &Out) {
+  SmallVector<OwningBinary<Binary>, 1> Binaries;
+  SmallVector<Slice, 1> Slices;
+  for (const auto &O : In.objects()) {
----------------
I'd up both of these to 2, since I imagine that's the most common universal binary size.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88400