[PATCH] D61927: [tools]Introduce llvm-lipo

[Michael Trent via Phabricator via llvm-commits]

mtrent added a comment.

A second thought on the topic of lipo compatibility. libObject is probably reasonable if your primary interest is MH_OBJECT files. There exist a number of final-linked-images that libObject's fat parser will not currently understand; these are formats that cannot be produced by llvm tools today. I wonder if choosing a different name for the tool might give you some flexibility to "do things differently" than lipo. That said, I don't have a better name in mind. :)



================
Comment at: test/tools/llvm-lipo/verify-arch-macho-binary.test:3
+
+# RUN: llvm-lipo %t -verify_arch i386
+# RUN: llvm-lipo %t --verify_arch i386
----------------
smeenai wrote:
> lipo only supports this form of specifying flags (single dash, no equals). Do we want to support the others?
I don't see a problem with supporting "--" options here, from the point of view of lipo compatibility. I say, do it.

There are other ways where llvm-lipo is going to be an 'awkward fit' as a drop-in replacement for lipo. For example, lipo is a little unusual in that when -verify_arch is specified, all the remaining options are interpreted as architectures. So llvm-lipo interprets "-verify_arch x86_64 /bin/ls" as "verify /bin/ls has an arch for x86_64 in it" but lipo itself says [paraphrasing] "/bin/ls is not an architecture." Either you will type "input_file -verify_arch x86_64 i386" or you will type "-verify_arch x86_64 -verify_arch i386 input_file" (where input_file could appear anywhere), and whichever usage you specify will be specific either to "llvm-lipo" or the historical lipo.

Pragmatically I think you're going to want to get as close as you can, to avoid confusion. But recognize there are going to be differences if you want to use llvm's argument parsing.


================
Comment at: tools/llvm-lipo/llvm-lipo.cpp:33
+static cl::list<std::string> VerifyArch(
+    "verify_arch", cl::ZeroOrMore,
+    cl::desc(
----------------
smeenai wrote:
> Shouldn't this be OneOrMore?
No. -verify_arch is one of several mutually-exclusive lipo commands. You cannot specify both -verify_arch and -info for example. So ZeroOrMore is correct, and the llvm-lipo command driver is going to have to enforce this exclusivity. 


================
Comment at: tools/llvm-lipo/llvm-lipo.cpp:95
+    for (StringRef Arch : VerifyArch)
+      if (O->getArch() != Triple(Arch).getArch())
+        reportError(Twine(O->getFileName()) + " does not contain " + Arch);
----------------
smeenai wrote:
> `Triple(Arch).getArch()` is used a bunch. Should we save it to a variable?
Do you mean "O->getArch()"? I agree, if so.


================
Comment at: tools/llvm-lipo/llvm-lipo.cpp:96
+      if (O->getArch() != Triple(Arch).getArch())
+        reportError(Twine(O->getFileName()) + " does not contain " + Arch);
+  } else {
----------------
the lipo -verify_arch command does not print an error if the command is able to run successfully, as it's primarily for using with shell scripts and Makefiles. Is a verbose message really necessary here? If less, please implement a -non_verbose option.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61927