[PATCH] D84770: [llvm-libtool-darwin] Add support for -arch_only

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:110
+        std::errc::invalid_argument,
+        "invalid architecture: %s\nValid architecture names are %s",
+        ArchitectureName.str().c_str(), OS.str().c_str());
----------------
We tend to avoid explicit new lines in error messages. They can make it non-obvious that output belongs to the error message, when stderr and stdout are piped to the same location, which can be confusing. You can probably just have this as a single sentence (i.e. "invalid architecture 'foo': valid architecture names are ..."). Note that I also added the `ArchitectureName` to the message, since that gives a little more context as to what is wrong.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:126-128
+  // Currently LLVM doesn't have the constant MachO::CPU_SUBTYPE_ARM64_V8.
+  // When the constant is added, following modification needs to be done for the
+  // case of MachO::CPU_TYPE_ARM64 below:
----------------
sameerarora101 wrote:
> sameerarora101 wrote:
> > jhenderson wrote:
> > > Rather than a comment that will be forgotten about and/or ignored, could you not just add the constant?
> > Is it as simple as adding a constant? Nothing else needs to be done?
> > 
> > Here is a potential place where I think the constant should go (under MachO.h):
> > ```
> > enum CPUSubTypeARM {
> >   CPU_SUBTYPE_ARM_ALL = 0,
> >   CPU_SUBTYPE_ARM_V4T = 5,
> >   CPU_SUBTYPE_ARM_V6 = 6,
> >   CPU_SUBTYPE_ARM_V5 = 7,
> >   CPU_SUBTYPE_ARM_V5TEJ = 7,
> >   CPU_SUBTYPE_ARM_XSCALE = 8,
> >   CPU_SUBTYPE_ARM_V7 = 9,
> >   //  unused  ARM_V7F     = 10,
> >   CPU_SUBTYPE_ARM_V7S = 11,
> >   CPU_SUBTYPE_ARM_V7K = 12,
> >   CPU_SUBTYPE_ARM_V6M = 14,
> >   CPU_SUBTYPE_ARM_V7M = 15,
> >   CPU_SUBTYPE_ARM_V7EM = 16
> > };
> > 
> > enum CPUSubTypeARM64 {
> >   CPU_SUBTYPE_ARM64_ALL = 0,
> >   CPU_SUBTYPE_ARM64E = 2,
> > };
> > 
> > enum CPUSubTypeARM64_32 { CPU_SUBTYPE_ARM64_32_V8 = 1 };
> > ```
> > However, I am also unsure of the RHS value that needs to be assigned to the constant. Any ideas?
> Added diff for constant V8 here: D85041
I think your other patch is fine. It may be beneficial to look at the history of the file you're modifiying to see what else got changed when other subtypes were added.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:249
+    if (ArchiveOrError) {
+      consumeError(MachOObjOrErr.takeError());
+      if (Error E = processArchive(Members, **ArchiveOrError, FileName, C))
----------------
sameerarora101 wrote:
> jhenderson wrote:
> > Why are you throwing away this error rahter than reporting it earlier?
> So a universal member can either be a `MachOObjectFile` or an `Archive` - so we need to try casting the member in both ways. First I try casting it as an object file. If that gives an error, it might be the case that the member is an Archive. Now, if I succeed on casting the member as an Archive, then I can throw away the error that I received from casting the member as an object file as the member was an archive and should not have been casted as an object file in first place.
> 
> `llvm-lipo` also seems to be following this design (under `printBinaryArchs`):
> ```
> for (const auto &O : UO->objects()) {
>       Expected<std::unique_ptr<MachOObjectFile>> MachOObjOrError =
>           O.getAsObjectFile();
>       if (MachOObjOrError) {
>         OS << Slice(MachOObjOrError->get()).getArchString() << " ";
>         continue;
>       }
>       Expected<std::unique_ptr<Archive>> ArchiveOrError = O.getAsArchive();
>       if (ArchiveOrError) {
>         consumeError(MachOObjOrError.takeError());
>         OS << Slice(ArchiveOrError->get()).getArchString() << " ";
>         continue;
>       }
>       consumeError(ArchiveOrError.takeError());
>       reportError(Binary->getFileName(), MachOObjOrError.takeError());
>     }
> ```
Okay, makes a degree of sense. `consumeError` calls should really have a comment explaining why it's safe to throw them away though, so please add one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84770