Page MenuHomePhabricator

[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

Diff Detail

Repository
rL 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
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.

llvm/tools/llvm-lipo/llvm-lipo.cpp
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"

alexshap added inline comments.Jun 16 2019, 3:56 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
275 ↗(On Diff #204797)

+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 ↗(On Diff #204797)

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 ↗(On Diff #204797)

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

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.

alexshap added inline comments.Jun 19 2019, 4:50 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
300 ↗(On Diff #205180)

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
alexshap accepted this revision.Jun 21 2019, 12:06 PM
smeenai accepted this revision.Jun 21 2019, 2:20 PM

LGTM

llvm/tools/llvm-lipo/llvm-lipo.cpp
95 ↗(On Diff #205841)

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

106 ↗(On Diff #205841)

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

Closed by commit rL364107: [llvm-lipo] Implement -thin (authored by smeenai, committed by ). · Explain WhyJun 21 2019, 3:00 PM
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.