[PATCH] D85334: [llvm-libtool-darwin] Support universal outputs

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/universal-output.test:54-66
+## Check that the subtypes of cputype CPU_TYPE_ARM64 are stored in a fat file:
+# RUN: yaml2obj %s -o %t.arm64 -DTYPE=0x0100000C -DSUBTYPE=0x0 -DSTRING=_arm64all
+# RUN: yaml2obj %s -o %t.arm64e -DTYPE=0x0100000C -DSUBTYPE=0x2 -DSTRING=_arm64e
+# RUN: llvm-libtool-darwin -static -o %t.lib %t.arm64 %t.arm64e
+# RUN: llvm-objdump --all-headers %t.lib | \
+# RUN:   FileCheck %s --check-prefix=UNIVERSAL-MEMBERS-ARM64 -DFILE=%t.lib -DPREFIX=%basename_t.tmp --implicit-check-not=Archive
+
----------------
sameerarora101 wrote:
> for `CPU_TYPE_ARM64`, I cannot test whether the architecture is present with `llvm-lipo` as it says "unknown(...)" for `arm64e`.  So I did this check using `llvm-objdump`  (whether the constituent members are present inside the fat file or not). Is this ok? To allow lipo to recognize `arm64e` I would have to make a modification under `llvm/lib/Object/MachOObjectFile.cpp` :
> ```
>   case MachO::CPU_TYPE_ARM64:
>     switch (CPUSubType & ~MachO::CPU_SUBTYPE_MASK) {
>     case MachO::CPU_SUBTYPE_ARM64_ALL:
>       if (McpuDefault)
>         *McpuDefault = "cyclone";
>       if (ArchFlag)
>         *ArchFlag = "arm64";
>       return Triple("arm64-apple-darwin");
>     default:
>       return Triple();
>     }
> ```
>  Not sure what would go under `*McpuDefault = ...`  and `return Triple("...");` for the case of `arm64e`?
Apple hasn't fully upstreamed arm64e to LLVM yet. You can see their implementation of this at https://github.com/llvm/llvm-project-staging/blob/f0fd4f3af0acda5068acc5bbe10b3792c41bdfc4/llvm/lib/Object/MachOObjectFile.cpp#L2695-L2700.

I think it's fine to use objdump for this for now; just add a comment about why.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:355-356
+    for (const llvm::detail::DenseMapPair<
+             unsigned long, std::vector<llvm::NewArchiveMember,
+                                        std::allocator<llvm::NewArchiveMember>>>
+             &M : NewMembers) {
----------------
You want `uint64_t`, and you don't need the `std::allocator` member in the vector (that's the default).


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:123
 
+static uint64_t getCPUID(uint32_t CPUType, uint32_t CPUSubtype) {
+  switch (CPUType) {
----------------
sameerarora101 wrote:
> jhenderson wrote:
> > Looks to me like this function is only partially tested? What about the other three CPUs and the default case?
> Ok also added test for `CPU_TYPE_ARM64`,  `CPU_TYPE_X86_64` and the default case. `CPU_TYPE_ARM64_32` does not have a subtype so I didn't add a test for it.
Can you have two files with a CPUType that falls under the default case but different CPUSubtypes, and confirm that they get put in the same slice of the output file?


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:354
+    SmallVector<OwningBinary<Archive>, 2> OutputBinaries;
+    for (const auto &M : NewMembers) {
+      Expected<std::unique_ptr<MemoryBuffer>> OutputBufferOrErr =
----------------
jhenderson wrote:
> You should probably avoid `auto` here. It's not obvious from the immediate context what the type is.
I agree that it's not obvious, but the expanded type for this (and for iterators in general) is also extremely verbose :/ I thought the general guideline was to use `auto` for range based for loops for that reason. Perhaps we could stick with the `auto` in the loop and assign the members of the pair to variables in the loop? (It looks like we're only using the second member anyway.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85334