Page MenuHomePhabricator

[llvm-ar] Create thin archives with MRI scripts
ClosedPublic

Authored by gbreynoo on Jun 5 2019, 10:43 AM.

Details

Summary

This patch implements the "CREATE_THIN" MRI script command, allowing thin archives to be created via MRI scripts. It is not found in gnu-ar but as it has been useful to some of our customers, I thought it worth putting up for review.

Diff Detail

Repository
rL LLVM

Event Timeline

gbreynoo created this revision.Jun 5 2019, 10:43 AM

I guess there are no rules for MRI script compatibility -- but I wonder how much work we want to invest in improving MRI support beyond supporting people migrating from GNU ar (even there, MRI support is only added to support people migrating from a different librarian program...). This seems like a minor change to allow, but I just wanted to say that before you go crazy adding things :)

tools/llvm-ar/llvm-ar.cpp
999 ↗(On Diff #203180)

tiny nit: remove extra whitespace before the colon

1000 ↗(On Diff #203180)

Add LLVM_FALLTHROUGH; after this to indicate intentional fallthrough

1000 ↗(On Diff #203180)

If you have an MRI script like this:

createthin foo.a
...
save
create bar.a
...
save

Then bar.a ought be a normal archive (because it was not created with createthin), but since Thin is a global variable here, it will be a thin archive. Is that correct? (Can you add a test for that?) I think you need to avoid having a fall through, i.e. the case MRICommand::Create should explicitly set Thin = false, and likewise case MRICommand::CreateThin should explicitly set it to true.

Alternatively, another solution is to provide thin and regular (or notthin, 'fat', or whatever opposite name you want) that toggles the global state.

(LGTM other than the comment about Thin being global tho)

gbreynoo updated this revision to Diff 203325.Jun 6 2019, 3:53 AM

Changes after rupprecht's comments.

ruiu added a comment.Jun 6 2019, 3:59 AM

Looks like this patch doesn't still fix the issue that rupprecht pointed out. If an MRI script looks like this

createthin foo.a
...
save
create bar.a
...
save

doesn't thes second "create" create a thin archive?

gbreynoo marked an inline comment as done.Jun 6 2019, 4:21 AM
gbreynoo added inline comments.
tools/llvm-ar/llvm-ar.cpp
1000 ↗(On Diff #203180)

Unfortunately a limitation of the current implementation is that editing multiple archives in one MRI script is not supported. In the event CREATE (or now CREATETHIN) are called more than once the whole operation fails. I'm happy to separate the two cases but at the moment there will not be a behaviour change.

ruiu accepted this revision.Jun 6 2019, 4:44 AM

I'm not sure if I'm a right person to make a decision whether adding a new MRI command is OK or not, but looks like this new command makes sense, and no one seems to be opposing, so LGTM.

tools/llvm-ar/llvm-ar.cpp
1000 ↗(On Diff #203180)

Ah, thanks for the explanation. If that's the case this looks fine.

This revision is now accepted and ready to land.Jun 6 2019, 4:44 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2019, 6:18 AM