This is an archive of the discontinued LLVM Phabricator instance.

[llvm-lipo] Add support for archives
ClosedPublic

Authored by alexander-shaposhnikov on Sep 19 2019, 5:09 AM.

Details

Summary

Add support for creating universal binaries which can contain an archive.

Test plan: make check-all

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2019, 5:09 AM
compnerd added inline comments.Sep 20 2019, 9:01 AM
tools/llvm-lipo/llvm-lipo.cpp
217 ↗(On Diff #220841)

Can we use the calculateAlignment here rather than hardcoding this? Also, I know its not part of your change, but, could you name Alginment as P2Alignment to make it clear that this is a power-of-two alignment and not byte alignment.

220 ↗(On Diff #220841)

Please name this setP2Alignment.

228 ↗(On Diff #220841)

Can we just make CPUType, CPUSubType, Alignment` const members and make them public? We can set them properly during the constructor and avoid the unnecessary accessors.

246 ↗(On Diff #220841)

There are a couple of $ introduced in the comment.

251 ↗(On Diff #220841)

Trailing $ in the comment?

alexander-shaposhnikov marked an inline comment as done.Sep 20 2019, 12:32 PM
alexander-shaposhnikov added inline comments.
tools/llvm-lipo/llvm-lipo.cpp
228 ↗(On Diff #220841)

the thing is that archives those fields should be calculated first (extracted from the members of the archive), thus they can't be const, exposing them as a non-const public doesn't look good to me.

Address review comments

alexander-shaposhnikov marked 3 inline comments as done.Sep 20 2019, 1:54 PM
compnerd accepted this revision.Sep 20 2019, 3:27 PM
This revision is now accepted and ready to land.Sep 20 2019, 3:27 PM
smeenai accepted this revision.Sep 20 2019, 3:51 PM
smeenai added a subscriber: anushabasana.

LGTM

tools/llvm-lipo/llvm-lipo.cpp
161 ↗(On Diff #221110)

"to stores" -> "to store"

246 ↗(On Diff #220841)

Still have an $ here.

mtrent accepted this revision.Sep 23 2019, 12:48 PM

Logic looks fine. I have some concerns around alignment terminology that I suggest you address before committing.

tools/llvm-lipo/llvm-lipo.cpp
105 ↗(On Diff #221143)

A segment alignment has no strong correlation to the section alignment, and the notion that the segment alignment is the maximum alignment of its sections is not true. Maybe the simplest way to say this is:

"For compatibility with cctools lipo, a file's alignment is calculated as the minimum aligment of all segments. For object files, the file's alignment is the maximum alignment of its sections."

Part of the problem is that the method here is named "calculateSegmentAlignment" but that 1) doesn't accurately describe how lipo is using that value, and 2) also doesn't completely describe what the function is doing. But cctools also has both these problems :) You have an opportunity to call this function something like "calculateSliceAlignment" or "calculateFileAlignment" which may be objectively better ...

Be aware the segment computation is merely a guess, and it may not be exactly correct. But it's correct enough for lipo.

And be aware the behavior for object files may change in future versions of cctools.

alexander-shaposhnikov marked an inline comment as done.Sep 23 2019, 2:12 PM
alexander-shaposhnikov added inline comments.
tools/llvm-lipo/llvm-lipo.cpp
105 ↗(On Diff #221143)

You are right.
Thanks, will change it.

This revision was automatically updated to reflect the committed changes.