This is an archive of the discontinued LLVM Phabricator instance.

[llvm-lipo] Implement -info
ClosedPublic

Authored by anushabasana on Jul 12 2019, 2:46 PM.

Details

Summary

Prints architecture type of all input files.

Test Plan: check-llvm

Event Timeline

anushabasana created this revision.Jul 12 2019, 2:46 PM
compnerd added inline comments.Jul 14 2019, 6:41 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
270

OS is the naming convention for the output stream

286

I think it may be nicer to change L280-L286 to something like:

    OS << '\n';
    return;
  }

  auto *O = cast<MachOObjectFile>(Binary);
  OS << getArchString(*O) << " \n";
}
318

Similar to above, you can use continue in the universal case and unindent the else case (make the Binary->isMachO() an assertion, and drop the unreachable.

547

Is there validation being applied here? I think that you should add a test case for invalid input as well (that is, pass ELF files to lipo and ensure that we handle that gracefully (right now, it feels like we may just abort?).

llvm/tools/llvm-lipo/llvm-lipo.cpp
547

if I'm not mistaken readInputBinaries takes care of it, otherwise this would be reimplemented N times where N is the number of supported actions. Having an extra test of -info would not hurt though.

llvm/tools/llvm-lipo/llvm-lipo.cpp
270

+1

301

khm, the same effect (grouping universal/regular files) could be achieved a bit simpler, what would you say to the following (no need for std::string, raw_string_ostream, ThinOutputBuffer, ThinOutputStream, UniversalOutputBuffer, UniversalOutputStream, etc)

 
for (const auto &IB : InputBinaries)
    if (IB.getBinary()->isMachOUniversalBinary()) {
         outs() << "Architectures in the fat file: " << Binary->getFileName() 
                   << " are: ";
         printBinaryArchs(IB.getBinary(), outs());
    }
 for (const auto &IB : InputBinaries)
    if (IB.getBinary()->isMachO) {
         outs() << "Non-fat file: " << IB.getBinary()->getFileName()
                   << " is architecture: ";
         printBinaryArchs(IB.getBinary(), outs());
    }
llvm/tools/llvm-lipo/llvm-lipo.cpp
301

cc: @mtrent

  1. The wording (especially in the second case) appears to be very weird, maybe it's worth to change it and make it more human-readable ? (I suspect -info is not really widely used in scripts, so probably this incompatibility might not be a big issue).
  2. Maybe we don't even need to group anything here (for the same reasons)
anushabasana marked 7 inline comments as done.

Address review comments

anushabasana marked an inline comment as not done.Jul 15 2019, 10:23 AM
anushabasana added inline comments.
llvm/tools/llvm-lipo/llvm-lipo.cpp
301

I could remove the string and OS for Universal files and just pass in outs() to cut down there.
I originally had 2 for loops but I thought the multiple OS would be better so we would only have to do one pass over the input binaries.
I'm not sure what is more legible, I don't have a strong preference.

compnerd added inline comments.Jul 15 2019, 10:29 AM
llvm/tools/llvm-lipo/llvm-lipo.cpp
301

@alexshap - I think that I would rather that we go with stronger claims:

for (const auto &IB : InputBinaries) {
  Binary *B = IB.getBinary();
  assert(B->isMachO() && "expected MachO binary");
  outs() << "Non-fat file: " << B->getFileName() << " is architecture: ";
  printBinaryArchs(B, outs());
}
547

Ah, okay, I was trying to see where that was called, I guess I just failed to see it. Yes, a test would be good still.

llvm/tools/llvm-lipo/llvm-lipo.cpp
301

I don't think that performance is really a concern here (and in any case IO + allocations are more expensive than two "for" loops are), so I'd prefer to make this code as simple as possible,
in particular, get rid of these intermediate strings and buffers (I'm referring to ThinOutputBuffer, ThinOS, etc)

compnerd added inline comments.Jul 15 2019, 10:43 AM
llvm/tools/llvm-lipo/llvm-lipo.cpp
301

It seemed that this was confusing for @alexshap, what I meant is that this could be written as:

for (const auto &IB : InputBinaries) {
  if (IB.getBinary()->isMachOUniversalBinary()) {
    ...
  }
}
for (const auto &IB : InputBinaries) {
  if (!IB.getBinary()->isMachOUniversalBinary()) {
    ...
  }
}

which makes it more explicit that the two loops explicitly go over everything just filtering on a specific condition.

anushabasana marked 6 inline comments as done.

Removed extra OS

anushabasana added inline comments.Jul 15 2019, 11:16 AM
llvm/tools/llvm-lipo/llvm-lipo.cpp
547

I added test info-invalid.test

smeenai added inline comments.Jul 15 2019, 11:46 AM
llvm/tools/llvm-lipo/llvm-lipo.cpp
301

I've definitely seen -info be used in some scripts, particularly since -archs is a more recent invention, I think. I think it's best if we match lipo output here.

llvm/tools/llvm-lipo/llvm-lipo.cpp
301

ok, then let's leave it as is

llvm/tools/llvm-lipo/llvm-lipo.cpp
285
OS << getArchString(*cast<MachOObjectFile>(Binary))
This revision is now accepted and ready to land.Jul 15 2019, 12:11 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
505

in order to ...

anushabasana marked 6 inline comments as done.

Address review comments

llvm/test/tools/llvm-lipo/info.test
8

CHECK
CHECK-NEXT
CHECK-NEXT
...

(to verify that the order is correct)

anushabasana marked an inline comment as done.

Test file uses CHECK-NEXT instead of CHECK

This revision was automatically updated to reflect the committed changes.