This is an archive of the discontinued LLVM Phabricator instance.

[llvm-lipo] Implement -thin
ClosedPublic

Authored by anushabasana on Jun 14 2019, 9:45 AM.

Details

Summary

Creates thin output file of specified arch_type from the fat input file.

Test Plan: check-llvm

Event Timeline

anushabasana created this revision.Jun 14 2019, 9:45 AM
compnerd added inline comments.Jun 14 2019, 11:29 AM
llvm/test/tools/llvm-lipo/thin-executable-universal-binary.test
2

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.

llvm/tools/llvm-lipo/llvm-lipo.cpp
165

nit. s/to be specified//

178

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.)

276

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"

278

Since llmv-lipo is meant to be a drop-in replacement for lipo, this must be EXIT_FAILURE

286

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"

llvm/tools/llvm-lipo/llvm-lipo.cpp
278

+1

Added comments, fixed error messages, and print error for archive files.

anushabasana marked 7 inline comments as done.Jun 17 2019, 2:32 PM

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.

llvm/tools/llvm-lipo/llvm-lipo.cpp
178

Added an error clarifying that archive files are not yet supported, but we will file a bug to implement this.
We are focusing on object files in this primary implementation.

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.

anushabasana marked an inline comment as done.Jun 18 2019, 3:04 PM
anushabasana added inline comments.
llvm/tools/llvm-lipo/llvm-lipo.cpp
178

Bug 42310 - - llvm-lipo does not support archive files
https://bugs.llvm.org/show_bug.cgi?id=42310

mtrent requested changes to this revision.Jun 18 2019, 9:13 PM

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.

This revision now requires changes to proceed.Jun 18 2019, 9:13 PM
In D63341#1547398, @alexshap wrote:

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.

I agree with all these points.

llvm/tools/llvm-lipo/llvm-lipo.cpp
300

khm, this works only because the method "commit" has not failed in your tests,
"commit" should not be called twice here. Basically, the correct "pattern" is the following:

if (Error E = OutFileOrError.get()->commit())
    reportError(OutputFileName, std::move(E));

Supports -o for output flag. Fixed order of error checking.

anushabasana marked an inline comment as done.Jun 20 2019, 9:57 AM

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.

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.

Note: "llvm-lipo" does not work properly with certain Mach-O executables, such as those built with the -hideARM64 flag.

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.

Sounds good. Note that it's relevant for all lipo flags, not just '-thin'

mtrent accepted this revision.Jun 21 2019, 12:18 AM
This revision is now accepted and ready to land.Jun 21 2019, 12:18 AM
smeenai accepted this revision.Jun 21 2019, 2:20 PM

LGTM

llvm/tools/llvm-lipo/llvm-lipo.cpp
95

Nit: you can just move the function instead of adding a prototype.

104

Nit: StringRef should be enough to trigger the Twine construction and is more efficient here.

anushabasana marked an inline comment as done.

Fixed small review comments

This revision was automatically updated to reflect the committed changes.

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.

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.

D63735 implements the nicer error message for an invalid architecture.