Page MenuHomePhabricator

[llvm-lipo] Implement -create part 1
ClosedPublic

Authored by anushabasana on Jul 2 2019, 2:23 PM.

Details

Summary

Creates universal binary output file from input files. Currently uses hard coded value for alignment.
Want to get the create functionality approved before implementing the alignment function.

Test Plan: check-llvm

Diff Detail

Repository
rL LLVM

Event Timeline

anushabasana created this revision.Jul 2 2019, 2:23 PM

@mtrent We have implemented create with the -fat64 flag to specify a fat64binary, similar to cctools lipo.
Do we still need this flag, or can we always create a fat64 binary?
Another option would be to have some sort of auto detection to create a fat64 binary when the offset of a section exceeds 32 bits.

compnerd added inline comments.Jul 2 2019, 2:44 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
274 ↗(On Diff #207627)

I think that this can be replaced with a range based loop:

for (auto O : UO->objects()) {
  Expected<std::unique_ptr<MachOObjectFile>> BinaryOrError = O.getAsObjectFile();
  if (!BinaryOrError)
    reportError(InputBinary->getFileName(), BinaryOrError.takeError());
  outs() << getArchString(BinaryOrError.get()->get()) << “ “;
}
329 ↗(On Diff #207627)

I think that having a helper will help:

auto CPUIDForSlice = [](const Slice &S) -> uint64_t {
  return S.ObjectFile->getHeader().cputype << 32 | S.ObjectFile->getHeader().cpusubtype;
};
404 ↗(On Diff #207627)

I think that it is better to return a SmallVectorImpl<FatArchType>

alexshap added inline comments.Jul 2 2019, 3:41 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
404 ↗(On Diff #207627)

i think no, this would cause "slicing", SmallVector has 2 immediate bases (nonempty) https://llvm.org/doxygen/SmallVector_8h_source.html#837

mtrent added a comment.Jul 2 2019, 3:44 PM

@mtrent We have implemented create with the -fat64 flag to specify a fat64binary, similar to cctools lipo.
Do we still need this flag, or can we always create a fat64 binary?
Another option would be to have some sort of auto detection to create a fat64 binary when the offset of a section exceeds 32 bits.

Support for fat64 is still largely experimental, and the -fat64 flag is intentionally undocumented. I suggest omitting it from llvm for now, in case we decide the implementation needs to change before we roll the feature out ourselves. Otherwise, you run the risk of forking the Mach-O file format, which would be ... unfortunate.

smeenai added inline comments.Jul 3 2019, 11:52 AM
llvm/tools/llvm-lipo/llvm-lipo.cpp
329 ↗(On Diff #207627)

I think you'd still need to cast S.ObjectFile->getHeader().cputype to a uint64_t beforehand, otherwise you'll be shifting an int by 32 bits and get UB.

Removed fat64 flag and addressed other review comments.

anushabasana marked 5 inline comments as done.Jul 3 2019, 2:15 PM
alexshap accepted this revision.Jul 3 2019, 3:40 PM

I've added a couple of minor comments, but other than that - looks good to me) I'd wait for @mtrent 's approval as well

llvm/tools/llvm-lipo/llvm-lipo.cpp
255 ↗(On Diff #207889)

const Triple

271 ↗(On Diff #207889)

for (const auto &O : ... )

This revision is now accepted and ready to land.Jul 3 2019, 3:40 PM
alexshap added inline comments.Jul 3 2019, 3:43 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
440 ↗(On Diff #207889)

nit: const uint64_t

alexshap added inline comments.Jul 3 2019, 3:48 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
384 ↗(On Diff #207889)

I'd rename I -> O:

for (const auto &O : UO->objects())
smeenai added inline comments.Jul 3 2019, 3:55 PM
llvm/tools/llvm-lipo/llvm-lipo.cpp
328 ↗(On Diff #207889)

Nit: you shouldn't need the -> uint64_t; the return type should be inferred correctly.

333 ↗(On Diff #207889)

Nit: you could use try_emplace instead of insert to avoid the need for std::make_pair.

416 ↗(On Diff #207889)

Nit: I'd add a newline above this, to separate the error handling from the normal codepath.

anushabasana marked 7 inline comments as done.

Addressed minor review comments

Updated alignment hardcoding

anushabasana marked an inline comment as done.Jul 9 2019, 6:23 PM
anushabasana added inline comments.
llvm/tools/llvm-lipo/llvm-lipo.cpp
357 ↗(On Diff #208857)

@mtrent The alignment for all of the valid architectures falls under the hard coded segment above. Is it still useful to implement the calculateAlignment function by segments for unknown architecture types?

  • [llvm-lipo] Implement -info

Revert last update, was a mistake

This revision was automatically updated to reflect the committed changes.