Creates thin output file of specified arch_type from the fat input file.
Test Plan: check-llvm
|1 ↗||(On Diff #204797)|
Might be nice to have a comment about this explaining that the executable testing is not possible on Windows since all files are considered executable.
I'll have a look Monday / early next week. Meanwhile, here are some quick thoughts.
|165 ↗||(On Diff #204797)|
nit. s/to be specified//
|178 ↗||(On Diff #204797)|
not really sure what the right protocol for this is in LLVM-land. Should you file a bug in some LLVM-specific bug tracker? Or maybe should you print a warning/error saying a format is not (yet) supported?
I thought Archive files were SymbolicFiles and so they "should" be supported, but maybe you're using the wrong level of API? (I could be wrong ... just asking.)
|273 ↗||(On Diff #204797)|
Nit / optional. Since llmv-lipo is meant to be a drop-in replacement for lipo, this should ideally produce "input file (hello) must be a fat file when the -thin option is specified"
|275 ↗||(On Diff #204797)|
Since llmv-lipo is meant to be a drop-in replacement for lipo, this must be EXIT_FAILURE
|283 ↗||(On Diff #204797)|
Nit / optional. Since llmv-lipo is meant to be a drop-in replacement for lipo, this should ideally produce: "fat input file (<FILE NAME>) does not contain the specified architecture (<ARCH NAME>) to thin it to"
Regarding the error messages, should should they replicate cctools lipo exactly?
Currently have small differences like "no output file specified" vs "thin expects a single output file".
If so, I can create a separate diff to change all the error messages.
|178 ↗||(On Diff #204797)|
Added an error clarifying that archive files are not yet supported, but we will file a bug to implement this.
i think that we should not "replicate cctools lipo error messages exactly", ours could be better / more convenient or informative where it makes sense.
But in this particular case (see the comments above) I agree, including the names into the error message would be good!
Regarding exit codes - yes, it's important (for many reasons) to make sure that our exit codes are correct.
Would be nice if "llvm-lipo -thin" printed a reasonable error message before dumping usage. Apple lipo will print "missing argument to -thin option"
Another difference is llvm-lipo will not validate the arch until all arguments are received, whereas Apple lipo will validate archs immediately. ("lipo -arch asdf")
Would be nice if "llvm-lipo" printed a list of valid archs when the arch is invalid. Currently it prints "llvm-lipo: error: Invalid architecture: asdf" and it exits. Apple lipo prints "unknown architecture specification flag: asdf in specifying thin operation: -thin asdf. known architecture flags are: ..."
PLEASE FIX: llvm-lipo does not support using "-o" in place of "-output"
Note: "llvm-lipo" does not work properly with certain Mach-O executables, such as those built with the -hideARM64 flag.
|300 ↗||(On Diff #205180)|
khm, this works only because the method "commit" has not failed in your tests,
if (Error E = OutFileOrError.get()->commit()) reportError(OutputFileName, std::move(E));
Fixed most of these issues, but for printing a list of knows architectures I am planning to make a change in a separate diff that returns a list of valid architectures, since I will no longer be modifying llvm-lipo code.
For the MachO executables built with -hideARM64 flag, I filed a bug for MachOUniversalBinary: https://bugs.llvm.org/show_bug.cgi?id=42343.