Page MenuHomePhabricator

[llvm-lipo] Implement -archs
ClosedPublic

Authored by anushabasana on May 31 2019, 2:55 PM.

Details

Summary

Displays the architecture names of an input file.
Unknown architectures are represented by unknown(cputype,cpusubtype).

Test Plan: check-llvm

Diff Detail

Repository
rL LLVM

Event Timeline

anushabasana created this revision.May 31 2019, 2:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2019, 2:55 PM
compnerd added inline comments.May 31 2019, 3:10 PM
llvm/test/tools/llvm-lipo/archs-macho-binary-unknown.test
1 ↗(On Diff #202492)

Can you avoid the temporary using:

yaml2obj %s | llvm-lip - -archs | FileCheck %s

llvm/test/tools/llvm-lipo/archs-universal-binary-unknown.test
1 ↗(On Diff #202492)

Similar

3 ↗(On Diff #202492)

Nit: double space after "as"

llvm/test/tools/llvm-lipo/archs-universal-binary.test
1 ↗(On Diff #202492)

Similar

llvm/tools/llvm-lipo/llvm-lipo.cpp
56 ↗(On Diff #202492)

This is confusing. Where is this used?

154 ↗(On Diff #202492)

I don't think that this should be unreachable - if the action is unspecified, that can occur normally in the way that the user invokes the tool. Perhaps this should be a proper error?

216 ↗(On Diff #202492)

Why not just make the parameter an OwningBinary<Binary> rather than the ArrayRef and then assert that a single entry is provided?

227 ↗(On Diff #202492)

I don't think that this is necessarily unreachable - an invalid binary being given to the tool could hit this. This should be a proper error.

smeenai added inline comments.May 31 2019, 3:25 PM
llvm/test/tools/llvm-lipo/archs-macho-binary-unknown.test
3 ↗(On Diff #202492)

This should be worded better.

6 ↗(On Diff #202492)

Nit: add a newline above this.

llvm/test/tools/llvm-lipo/archs-macho-binary.test
10–11 ↗(On Diff #202492)

This one isn't specific to -archs; it should happen when combining any two flags.

There's an existing help-message.test. Let's rename that to something like command-line.test and move this check to there.

llvm/test/tools/llvm-lipo/archs-universal-binary-unknown.test
3 ↗(On Diff #202492)

Comment should be worded better.

llvm/tools/llvm-lipo/llvm-lipo.cpp
56 ↗(On Diff #202492)

Line 64 below references LIPO_##PREFIX, and for an option group the prefix is nullptr. You should add a comment.

135 ↗(On Diff #202492)

At this point, we should have a single arg inside ActionArgs, so we can just dispatch based on that instead of rechecking InputArgs. Something like

switch (ActionArgs[0]->getOption().getID()) {
case LIPO_verify_arch:
  ...
  break;
case LIPO_archs:
  ...
  break;
default:
  llvm_unreachable("unexpected action argument");
}
154 ↗(On Diff #202492)

Lines 123 through 133 should ensure we have a single action argument of the expected type. My switch statement suggestion above should make that structure clearer.

216 ↗(On Diff #202492)

Good idea. We'd probably also wanna take the parameter by const reference?

227 ↗(On Diff #202492)

readInputBinaries above should have ensured we have a binary in the correct format.

alexshap added inline comments.May 31 2019, 3:31 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
154 ↗(On Diff #202492)

llvm_unreachable is for internal logical errors, here if the specified command line arguments are incorrect we should make use of reportError, the tool should not crash.

alexshap added inline comments.May 31 2019, 3:35 PM
llvm/test/tools/llvm-lipo/archs-macho-binary-unknown.test
1 ↗(On Diff #202492)

i think lipo doesn't support this (i.e. cat a.o | lipo - -archs wouldn't work), if we need this feature, we can add a proper support in llvm-lipo in the future, but i think it's a bit beyond the scope of the current patch.

alexshap added inline comments.May 31 2019, 3:38 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
227 ↗(On Diff #202492)

i think here llvm_unreachable is appropriate since it should have been checked earlier, so here it's just a defensive assert (rather than do the same check and error reporting in multiple places)

alexshap added inline comments.May 31 2019, 3:44 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
216 ↗(On Diff #202492)

not sure it really matters, the thing is that in general lipo can have multiple input files,
different actions expect different number of input files, thus since in main() we dispatch handling of particular commands to different subroutines i think it's clear / consistent enough. I'd prefer to not to make that "top-level" code less readable, although it's simple either way so i don't insist.

alexshap added inline comments.May 31 2019, 3:49 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
227 ↗(On Diff #202492)

or convert it to assert

alexshap added inline comments.May 31 2019, 3:55 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
216 ↗(On Diff #202492)

discussed with @compnerd and we agreed on this, might be worth adding a comment in the code (in main) that the details/specifics of handling particular options (i.e. the number of expected input files) are inside the corresponding subroutines.

anushabasana marked an inline comment as done.

Address review comments

smeenai added inline comments.May 31 2019, 5:31 PM
llvm/test/tools/llvm-lipo/archs-macho-binary-unknown.test
1 ↗(On Diff #202492)

Seems like It Just Works™. I guess the command line parsing just handles it?

llvm/tools/llvm-lipo/llvm-lipo.cpp
208 ↗(On Diff #202515)

Nit: add a period at the end.

154 ↗(On Diff #202492)

I actually think this would be an internal logic error:

  • If the user hasn't specified exactly one action argument, we'll have errored out already.
  • The only action arguments we currently support are -verify_arch and -archs.
  • Therefore, if our option isn't one of them, something is pretty wrong with our program logic.

I'm okay with using reportError for this though.

216 ↗(On Diff #202492)

What did you have in mind for the comment? I feel like with the way the switch is structured right now it's pretty obvious that all details of handling a particular option are handled inside the function for it, and having a comment saying that seems unnecessary.

227 ↗(On Diff #202492)

I think llvm_unreachable is better here, since the assert equivalent would be an assert(0) anyway.

alexshap added inline comments.May 31 2019, 5:47 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
154 ↗(On Diff #202492)

imo it's not. llvm_unreachable wouldn't be the right choice here for the following reason: for example, if a user has misspelled a flag or option the tool should not crash with assert/llvm_unreachable, it should exit normally (report error and exit with non-zero code)

alexshap added inline comments.May 31 2019, 5:52 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
216 ↗(On Diff #202492)

I didn't / I don't have a strong opinion regarding comments. Saleem wanted one.

smeenai added inline comments.May 31 2019, 5:53 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
154 ↗(On Diff #202492)

I agree, but if the user had misspelled a flag or option, we would have also handled it already (we check for unknown arguments explicitly above).

Basically, in order to have gotten to this switch statement, we should have performed all our input validation already. If we're ending up with an unknown option here, that means something is wrong with our input validation logic.

smeenai added inline comments.May 31 2019, 5:54 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
154 ↗(On Diff #202492)

Basically, under which circumstances could the default case be reached? I believe that with our current code we should never be able to reach it.

alexshap added inline comments.May 31 2019, 6:06 PM
llvm/test/tools/llvm-lipo/archs-macho-binary-unknown.test
1 ↗(On Diff #202492)

it works with llvm-lipo because in https://llvm.org/doxygen/Binary_8cpp_source.html#l00092
MemoryBuffer::getFileOrSTDIN does the thing https://llvm.org/doxygen/MemoryBuffer_8cpp_source.html#l00143

llvm/tools/llvm-lipo/llvm-lipo.cpp
154 ↗(On Diff #202492)

depending on how good our validation will be in the future, this invariant ("unreachable") might be preserved, if one day this invariant gets violated - okay, the tool will start crashing on some incorrectly specified flags. So this was more like a general consideration what the tool should do if a flag or a combination of flags has been specified incorrectly, in particular, it should not crash.

alexshap added inline comments.May 31 2019, 6:09 PM
llvm/test/tools/llvm-lipo/archs-macho-binary-unknown.test
1 ↗(On Diff #202492)

the only downside which i see - if smb decides to grab the shell command and replace llvm-lipo with lipo it will stop working

smeenai added inline comments.May 31 2019, 6:58 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
154 ↗(On Diff #202492)

Sounds good, I can live with that.

Small changes from review comments

Ping

llvm/tools/llvm-lipo/llvm-lipo.cpp
216 ↗(On Diff #202492)

what sort of comment did you have in mind?

alexshap accepted this revision.Jun 6 2019, 2:14 PM

I think this looks okay / relatively small change anyway, i don't want to block it anymore, if there are any extra comments I think we can always either revert the diff or address them post-commit.

This revision is now accepted and ready to land.Jun 6 2019, 2:14 PM
mtrent requested changes to this revision.Jun 6 2019, 2:41 PM

I did not download or run this code, but I believe it does not correctly divine the Arch flag from a given Mach-O binary. This is straight-forward to fix.

llvm/tools/llvm-lipo/llvm-lipo.cpp
209 ↗(On Diff #203267)

The problem here is that there are Mach-O Arch types that are identifiable by cputype and cpusubtype, such as "x86_64h" and "armv7k". The general Triple API does not seem to support this; so Mach-O tools get this from the MachOObjectFile or from the MachOObjectFile::getArchTriple.

Since the code calling printArchFromCPUType has a MachOObjectFile (Obj), I would simply pass that MachOObjectFile in, ask it for its Triple (Obj->getArch()), and then handle an unknown triple. But you could remove the dependency on a MachOObjectFile by replacing your call to static MachOObjectFile::getArch() with a call to static MachOObjectFile::getArchTriple(): given the cputype and cpusubtype it will return the ArchFlag for you. But that's kind of backwards ...

224 ↗(On Diff #203267)

See my note above; if it were me I'd pass Obj in here, and maybe rename the "printArchFromCPUType" to "printArchOrUnknown".

This revision now requires changes to proceed.Jun 6 2019, 2:41 PM
alexshap added inline comments.Jun 6 2019, 3:18 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
209 ↗(On Diff #203267)

I missed this, thanks!
I think we need a test for this as well

anushabasana marked an inline comment as not done.

Address review comments

mtrent accepted this revision.Jun 7 2019, 11:45 AM
mtrent added inline comments.
llvm/tools/llvm-lipo/llvm-lipo.cpp
207 ↗(On Diff #203578)

Ugh. I made a note to fix this trailing space in a future release. Sorry for the trouble, past and future.

This revision is now accepted and ready to land.Jun 7 2019, 11:45 AM

Added Expected<> error check

Closed by commit rL362840: [llvm-lipo] Implement -archs (authored by smeenai, committed by ). · Explain WhyJun 7 2019, 1:44 PM
This revision was automatically updated to reflect the committed changes.