This is an archive of the discontinued LLVM Phabricator instance.

[llvm-libtool-darwin] Allow flattening archives
ClosedPublic

Authored by sameerarora101 on Jul 9 2020, 3:55 PM.

Details

Summary

Add support for flattening archives while creating static libraries. As
per cctools' libtool's behavior, llvm-libtool-darwin does not flatten
archives recursively. Depends on D83002.

Diff Detail

Event Timeline

sameerarora101 created this revision.Jul 9 2020, 3:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2020, 3:55 PM
sameerarora101 retitled this revision from [llvm-libtool-darwin] Allow flatenning archives to [llvm-libtool-darwin] Allow flattening archives.Jul 9 2020, 4:15 PM

Updating type and comments

verifyDarwinObject -> verifyMachOObject

MachO -> Mach-O in comments

jhenderson added inline comments.Jul 15 2020, 1:38 AM
llvm/test/tools/llvm-libtool-darwin/invalid-archive.test
4 ↗(On Diff #277423)

Instead of using a separate Inputs file, you might want to consider using echo to create the input file on the fly. That has the advantage of keeping the test self-contained - you don't have to go looking elsewhere to understand what exactly is going on.

9 ↗(On Diff #277423)

I take it this behaviour is taken from the original libtool?

12 ↗(On Diff #277423)

Are you deliberately trying to add the archive symbol index here? That is done implicitly by llvm-ar anyway, so cr is sufficient.

17 ↗(On Diff #277423)

This needs fixing (you are deleting the archive from the previous case, not the two archives from this case).

19–20 ↗(On Diff #277423)

Ditto.

27–31 ↗(On Diff #277423)

Consider using a different archive file name here, so that you don't overwrite the archive from the earlier case. This can be useful in some debugging situations.

llvm/test/tools/llvm-libtool-darwin/valid-archive.test
1 ↗(On Diff #277423)

I'd rename this test archive-flattening.test. You should also fold in the invalid cases into it, as there's no need to keep them separate.

7 ↗(On Diff #277423)

What's the reasoning for deleting %t.lib explicitly here?

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
98

Don't use auto here. It's not obvious what the type is.

sameerarora101 marked 11 inline comments as done.Jul 15 2020, 8:00 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-libtool-darwin/invalid-archive.test
9 ↗(On Diff #277423)

Yup, cctools' libtool also throws an error for this case

llvm/test/tools/llvm-libtool-darwin/valid-archive.test
7 ↗(On Diff #277423)

Oh yeah, I don't need to do that as llvm-libtool-darwin overwrites it. Removed now. Thanks

sameerarora101 marked 2 inline comments as done.

Merge tests into archive-flattening.test

jhenderson accepted this revision.Jul 16 2020, 12:37 AM

LGTM, with the test case reordering. If you want to wait for D83834 too, then that's fine by me.

llvm/test/tools/llvm-libtool-darwin/archive-flattening.test
7

I'd reverse the order - put the positive cases before the negative ones.

8–9

It's not committed at the time of writing, but D83834 adds a utility to split an input file into smaller pieces. It might be an even nicer way than using echo to emit the archive.

This revision is now accepted and ready to land.Jul 16 2020, 12:37 AM
sameerarora101 marked 3 inline comments as done.Jul 16 2020, 7:13 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-libtool-darwin/archive-flattening.test
8–9

Ok, I can wait for it (while I also need to wait for few others to review this diff) Thanks!

sameerarora101 marked an inline comment as done.

Putting positive tests before negative ones.

smeenai accepted this revision.Jul 20 2020, 10:21 AM

LGTM

llvm/test/tools/llvm-libtool-darwin/archive-flattening.test
28

The archive format is more properly referred to as "Darwin" (at least that's the terminology LLVM uses); the object file format is Mach-O.

sameerarora101 marked 2 inline comments as done.Jul 20 2020, 12:20 PM
sameerarora101 added inline comments.
llvm/test/tools/llvm-libtool-darwin/archive-flattening.test
28

ok, got it! Thanks

sameerarora101 marked an inline comment as done.

Mach-O -> Darwin for archive format in comments

smeenai added inline comments.Jul 20 2020, 12:57 PM
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
97–102

Thinking through this a little more, Err is passed to the iterator to indicate iteration issues, so you should be checking it inside the loop in each iteration (probably before you call addChildMember). You still need to do an explicit check of Err afterward, so that in the case where the loop didn't iterate at all, the Error is still considered checked (it's guaranteed to be a success in that case, but you still need to do an explicit boolean check to mark it as checked). Perhaps the following would suffice for that? I'm not sure what pattern other parts of the codebase use for this.

static_cast<bool>(Err);
smeenai added inline comments.Jul 20 2020, 3:10 PM
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
97–102

Ah, never mind. The iteration will stop in case of an error, so having the error check after the loop is perfectly fine. (See llvm/include/llvm/ADT/fallible_iterator.)

jhenderson added inline comments.Jul 21 2020, 12:53 AM
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
97–102

I had basically this same thought originally too, but this is the pattern for looping through archive members elsewhere, so I realised it was safe.

This revision was automatically updated to reflect the committed changes.