[llvm-ar] Fix thin archive being wrongly converted to a full archive

[gbreynoo]

Summary

When using the L option to quick append a full archive to a thin archive, the thin archive was being wrongly converted to a full archive. I've fixed the issue and added a check for it in thin-to-full-archive.test and expanded some tests.

[gbreynoo]

The following tests have been modified:

• flatten-thin-archive.test now explicitly checks that %t.a is thin after the other archives have been added.
• full-to-thin-archive.test uses Inputs/a.txt as an input rather than itself as spurious passes could have occurred when looking for strings contained in the archive file. Corrected a comment, added a test case for an existing archive and a case attempting to convert an existing archive.
• thin-to-full-archive.test now tests the contents to ensure the thin archives member have been added and not the archive itself. Added a test case for an existing archive and a case attempting to convert an existing archive.

[jhenderson]

llvm/test/tools/llvm-ar/full-to-thin-archive.test

11 ↗

(On Diff #420553)

15 ↗

(On Diff #420553)

You've used the term "full archive" here and in the comment above, but simply "archive" in other places. Be consistent, use one version. FWIW, the error message is "regular archive".

24 ↗

(On Diff #420553)

I recommend checking the error message here.

llvm/test/tools/llvm-ar/thin-to-full-archive.test

28 ↗

(On Diff #420553)

This isn't used anywhere. Did you mean FULL-NOT? If so, I'd recommend instead adding --implicit-check-not=thin.a to the FileCheck command, since NOT matches are order-sensitive.

32 ↗

(On Diff #420553)

Also elsewhere with other comments.

llvm/tools/llvm-ar/llvm-ar.cpp

1031–1032

I've not looked at the full code paths, but at an educated guess, moving this code here will cause this error to occur in other cases which don't add new members, such as t operations? Is that desirable (I'm inclined to say so, but at least want to flag up the possibility of an issue)?

[gbreynoo]

Corrected comments including replacing use of "full archive" with "regular archive" as suggested. To remain consistent two tests have been renamed to use the term "regular". The checks for "thinness" in llvm-ar.cpp now only applies for write commands.

[gbreynoo]

llvm/test/tools/llvm-ar/thin-to-full-archive.test

28 ↗

(On Diff #420553)

Thanks, I've made the change as suggested.

llvm/tools/llvm-ar/llvm-ar.cpp

1031–1032

Good catch, this has been changed to only check on write commands.

[MaskRay]

Perhaps precommit the test updates with renaming and -T => --thin.

[gbreynoo]

Good idea @MaskRay, I've split that into https://reviews.llvm.org/D123778 and will update this one when D123778 is complete.

[jhenderson]

In D123142#3451306, @gbreynoo wrote:

Good idea @MaskRay, I've split that into https://reviews.llvm.org/D123778 and will update this one when D123778 is complete.

There is no particular reason to wait - you could rebase this patch on top of that one, so that this patch doesn't include the other changes that will have already landed. To indicate that this patch isn't intended as a standalone patch, you then use the "Edit Related Revisions" UI option to the right of hte patch description.

Holding off approving until that's happen, as it would be easier for me to review exactly what this patch is finally doing.

llvm/tools/llvm-ar/llvm-ar.cpp

1031–1032

Is there a test case you could add that would have failed with the previous version of this patch?

[gbreynoo]

Updated after https://reviews.llvm.org/D123778 was accepted and committed.

[gbreynoo]

llvm/tools/llvm-ar/llvm-ar.cpp

1031–1032

Added to regular-to-thin-archive.test.

[jhenderson]

llvm/test/tools/llvm-ar/regular-to-thin-archive.test

28

Nit: I'd delete this blank line, since the line before is strongly tied to the lines immediately after.

llvm/test/tools/llvm-ar/thin-to-regular-archive.test

34

This test is failing the premerge checks, presumably because of the out-of-date %tfull2.a name.

36

Nit: consistency between singular versus plural.

[gbreynoo]

Fixed failing test and @jhenderson's other points.

[jhenderson]

LGTM

llvm/test/tools/llvm-ar/flatten-thin-archive.test

 # XFAIL: system-aix
 ## This test creates a thin archive that contains a thin archive, a regular
 ## archive, and a file.
 ##
 ## The inner thin archive should be flattened, but the regular archive should
 ## not. The order of members in the archive should match the input order, with
 ## flattened members appearing together.
 
 # RUN: touch %t-a.txt %t-b.txt %t-c.txt %t-d.txt %t-e.txt
 # RUN: rm -f %t-a-plus-b.a %t-d-plus-e.a %t.a
 # RUN: llvm-ar rcs --thin %t-a-plus-b.a %t-a.txt %t-b.txt
 # RUN: llvm-ar rcs %t-d-plus-e.a %t-d.txt %t-e.txt
 # RUN: llvm-ar rcs --thin %t.a %t-a-plus-b.a %t-c.txt %t-d-plus-e.a
-# RUN: llvm-ar t %t.a | FileCheck %s
+# RUN: FileCheck --input-file=%t.a %s
 
+# CHECK:      !<thin>
 # CHECK:      a.txt
 # CHECK-NEXT: b.txt
 # CHECK-NEXT: c.txt
 # CHECK-NEXT: -d-plus-e.a

llvm/test/tools/llvm-ar/regular-to-thin-archive.test

 # XFAIL: system-aix
 ## Test regular archives do not convert to thin archives.
 
 # RUN: rm -rf %t && mkdir -p %t
-# RUN: llvm-ar qc %t/archive.a %s
+# RUN: llvm-ar qc %t/archive.a %S/Inputs/a.txt
 # RUN: not llvm-ar q --thin %t/archive.a %s 2>&1 | FileCheck %s
 # RUN: not llvm-ar r --thin %t/archive.a %s 2>&1 | FileCheck %s
 
 # CHECK: error: cannot convert a regular archive to a thin one
 
-## Test that you can add a regular archive's contents to a thin archive with 'L'
-# RUN: llvm-ar -qcL --thin %t/thin.a %t/archive.a
-# RUN: FileCheck --check-prefixes=THIN --input-file=%t/thin.a %s
+## Test that you can add a regular archive to a new thin archive with 'L'.
+# RUN: llvm-ar -qcL --thin %t/thin1.a %S/Inputs/b.txt %t/archive.a
+# RUN: FileCheck --check-prefixes=THIN --input-file=%t/thin1.a %s
 
-THIN: !<thin>
+## Test that you can add a regular archive to an existing thin archive with 'L'.
+# RUN: llvm-ar -qc --thin %t/thin2.a %S/Inputs/b.txt
+# RUN: llvm-ar -qL --thin %t/thin2.a %t/archive.a
+# RUN: FileCheck --check-prefixes=THIN --input-file=%t/thin2.a %s
+
+# THIN: !<thin>
+# THIN: archive.a
+
+## Test archives do not convert to thin archives with use of 'L' and '--thin'.
+# RUN: not llvm-ar -qL --thin %t/archive.a %t/thin2.a 2>&1 | FileCheck %s
+
+## Test that a regular archive can be read despite use of --thin 
+# RUN: llvm-ar -t --thin %t/archive.a
+# RUN: FileCheck --check-prefixes=REGULAR --input-file=%t/archive.a %s
+# REGULAR: !<arch>

llvm/test/tools/llvm-ar/thin-to-regular-archive.test

 # XFAIL: system-aix
 ## Test thin archives do not siletly convert to regular archives on write.
 
-# RUN: rm -f %tthin.a %tregular.a 
+# RUN: rm -f %tthin.a %tregular1.a %tregular2.a
 
 # RUN: llvm-ar -rc --thin %tthin.a %S/Inputs/a.txt
 # RUN: FileCheck --check-prefixes=THIN --input-file=%tthin.a %s
 
 # RUN: llvm-ar -q %tthin.a %S/Inputs/b.txt
 # RUN: FileCheck --check-prefixes=THIN --input-file=%tthin.a %s
 
 # RUN: llvm-ar -r %tthin.a %S/Inputs/c.txt
 # RUN: FileCheck --check-prefixes=THIN --input-file=%tthin.a %s
 
 # RUN: llvm-ar -am %S/Inputs/a.txt %tthin.a %S/Inputs/c.txt
 # RUN: FileCheck --check-prefixes=THIN --input-file=%tthin.a %s
 
 # RUN: llvm-ar -d %tthin.a %S/Inputs/c.txt
 # RUN: FileCheck --check-prefixes=THIN --input-file=%tthin.a %s
 
 # THIN: !<thin>
 
-## Test that you can add a thin archive's contents to a regular archive with 'L'
-# RUN: llvm-ar -qcL %tregular.a %tthin.a
-# RUN: FileCheck --check-prefixes=REGULAR --input-file=%tregular.a %s
+## Test that you can add a thin archive's contents to a regular archive with 'L'.
+# RUN: llvm-ar -qcL %tregular1.a %tthin.a
+# RUN: FileCheck --check-prefixes=REGULAR --input-file=%tregular1.a --implicit-check-not=thin.a %s
+
+# REGULAR: !<arch>
+# REGULAR: a.txt
+# REGULAR: b.txt
+
+## Test that you can add a thin archive's contents to an existing regular archive with 'L'.
+# RUN: llvm-ar -q %tregular2.a %S/Inputs/d.txt
+# RUN: llvm-ar -qL %tregular2.a %tthin.a
+# RUN: FileCheck --check-prefixes=REGULAR --input-file=%tregular2.a %s
 
-REGULAR: !<arch>
+## Test that thin archives do not convert to regular archives with 'L' and no use of '--thin'.
+# RUN: llvm-ar -qL %tthin.a %tregular2.a
+# RUN: FileCheck --check-prefixes=THIN --input-file=%tthin.a %s

llvm/tools/llvm-ar/llvm-ar.cpp

static void performReadOperation(ArchiveOperation Operation,
   for (StringRef Name : Members)
     WithColor::error(errs(), ToolName) << "'" << Name << "' was not found\n";
   exit(1);
 }
 
 static void addChildMember(std::vector<NewArchiveMember> &Members,
                            const object::Archive::Child &M,
                            bool FlattenArchive = false) {
-  if (Thin && !M.getParent()->isThin())
-    fail("cannot convert a regular archive to a thin one");
-
-  // Avoid converting an existing thin archive to a regular one.
-  if (!AddLibrary && M.getParent()->isThin())
-    Thin = true;
-
   Expected<NewArchiveMember> NMOrErr =
       NewArchiveMember::getOldMember(M, Deterministic);
   failIfError(NMOrErr.takeError());
   // If the child member we're trying to add is thin, use the path relative to
   // the archive it's in, so the file resolves correctly.
   if (Thin && FlattenArchive) {
     StringSaver Saver(Alloc);
     Expected<std::string> FileNameOrErr(M.getName());
static object::Archive::Kind getKindFromMember(const NewArchiveMember &Member) {
 
   return getDefaultForHost();
 }
 
 static void performWriteOperation(ArchiveOperation Operation,
                                   object::Archive *OldArchive,
                                   std::unique_ptr<MemoryBuffer> OldArchiveBuf,
                                   std::vector<NewArchiveMember> *NewMembersP) {
+  if (OldArchive) {
+    if (Thin && !OldArchive->isThin())
+      fail("cannot convert a regular archive to a thin one");
+
+    if (OldArchive->isThin())
+      Thin = true;
+  }
+
   std::vector<NewArchiveMember> NewMembers;
   if (!NewMembersP)
     NewMembers = computeNewArchiveMembers(Operation, OldArchive);
 
   object::Archive::Kind Kind;
   switch (FormatType) {
   case Default:
     if (Thin)
static int performOperation(ArchiveOperation Operation,
   if (!EC) {
     Expected<std::unique_ptr<object::Archive>> ArchiveOrError =
         object::Archive::create(Buf.get()->getMemBufferRef());
     if (!ArchiveOrError)
       failIfError(ArchiveOrError.takeError(),
                   "unable to load '" + ArchiveName + "'");
 
     std::unique_ptr<object::Archive> Archive = std::move(ArchiveOrError.get());
     if (Archive->isThin())
       CompareFullPath = true;
     performOperation(Operation, Archive.get(), std::move(Buf.get()),
                      NewMembers);
     return 0;
   }
 
   assert(EC == errc::no_such_file_or_directory);
 
   if (!shouldCreateArchive(Operation)) {