Page MenuHomePhabricator

[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

Diff Detail

Repository
rL 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 ↗(On Diff #209608)

OS is the naming convention for the output stream

286 ↗(On Diff #209608)

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

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

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

alexshap added inline comments.Jul 14 2019, 6:49 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
547 ↗(On Diff #209608)

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.

alexshap added inline comments.Jul 15 2019, 4:35 AM
llvm/tools/llvm-lipo/llvm-lipo.cpp
270 ↗(On Diff #209608)

+1

301 ↗(On Diff #209608)

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());
    }
alexshap added inline comments.Jul 15 2019, 7:51 AM
llvm/tools/llvm-lipo/llvm-lipo.cpp
301 ↗(On Diff #209608)

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

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

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

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.

alexshap added inline comments.Jul 15 2019, 10:34 AM
llvm/tools/llvm-lipo/llvm-lipo.cpp
301 ↗(On Diff #209608)

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

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

I added test info-invalid.test

smeenai added inline comments.Jul 15 2019, 11:46 AM
llvm/tools/llvm-lipo/llvm-lipo.cpp
301 ↗(On Diff #209608)

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.

alexshap added inline comments.Jul 15 2019, 12:07 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
301 ↗(On Diff #209608)

ok, then let's leave it as is

alexshap added inline comments.Jul 15 2019, 12:10 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
283 ↗(On Diff #209917)
OS << getArchString(*cast<MachOObjectFile>(Binary))
alexshap accepted this revision.Jul 15 2019, 12:11 PM
This revision is now accepted and ready to land.Jul 15 2019, 12:11 PM
alexshap added inline comments.Jul 15 2019, 12:22 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
495 ↗(On Diff #209917)

in order to ...

anushabasana marked 6 inline comments as done.

Address review comments

alexshap added inline comments.Jul 15 2019, 2:21 PM
llvm/test/tools/llvm-lipo/info.test
7 ↗(On Diff #209943)

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

Closed by commit rL366772: [llvm-lipo] Implement -info (authored by smeenai, committed by ). · Explain WhyJul 22 2019, 5:41 PM
This revision was automatically updated to reflect the committed changes.