[llvm-ar] Fix MRI ADDLIB command when used with thin archives

[gbreynoo]

Summary

We did not properly handle using CREATETHIN in an MRI script and attempting to use ADDLIB to add the contents of a regular archive. This fix outputs a meaningful error message in this case and provides some more testing.

[MaskRay]

An alternative way organizing MRI tests is to place them into regular-to-thin-archive.test and thin-to-regular-archive.test , but a dedicated test file for MRI & Thin looks fine, too.

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

1–3

My personal feeling is that placing rm, split-file, mkdir on one line is fine (many tests do this). It's still clear.

1–3

# UNSUPPORTED: system-aix

AIX doesn't use <arch>, and I am unsure how thin archives work for them.

9–10

ditto below

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

1068

grammar mistakes in "a regular archives contents"

I'd suggest cannot merge a regular archive into a thin archive, but happy to use one @jhenderson prefers.

[jhenderson]

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

1–3

Alternatively, use a --format option in the llvm-ar invocations, but it's probably simpler to use the # UNSUPPORTED as @MaskRay suggests.

13

I'd give this check an explicit prefix, rather than relying on the default, since you have other prefixes elsewhere.

18

This will only show that delete.o is not the last member of the archive. It would probably be more advisable to use FileCheck --implicit-check-not=delete.o (I know that this was here before, but since you're basically rewriting the test, you might as well improve it whilst here).

23

What is the purpose of this line? All it does is show that elf.o appears somewhere in the archive, and doesn't confirm whether or not a) there are any other members and b) whether the name is a full path (because FileCheck looks for substrings unless told to do otherwise).

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

1068

I'm not a fan of the word "merge" here, because it has implications around the old regular archive no longer existing. I think the original is closer to being correct, but missing an apostrophe: "cannot add a regular archive's contents to a thin archive".

[gbreynoo]

Fixed whitespace, added UNSUPPORTED: system-aix, placed rm, split-file, mkdir on one line, added explicit prefix and improved error message.

[gbreynoo]

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

23

This was just to check the archive was regular, I've removed the unneeded check for elf.o

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

1068

I went for adding the apostrophe.

[jhenderson]

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

16

Perhaps worth adding --match-full-lines to the FileCheck output, to ensure your path is as expected here? (e.g. this will also match addlib/elf.o/ or myelf.other.o etc as things stand)

[gbreynoo]

Updated with use of --match-full-lines.

[jhenderson]

LGTM!

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

-RUN: rm -rf %t && mkdir -p %t/addlib/
+# UNSUPPORTED: system-aix
+# RUN: rm -rf %t && mkdir -p %t/addlib/ && split-file %s %t
+# RUN: cd %t
 
-RUN: yaml2obj %S/Inputs/elf.yaml -o %t/elf.o
-RUN: cp %t/elf.o %t/addlib/elf.o
-RUN: cp %t/elf.o %t/delete.o
-
-RUN: cd %t && llvm-ar rTc addlib/addlib.ar addlib/elf.o
-
-RUN: echo "createthin %t/archive.ar" > %t/mri.script
-RUN: echo "addmod elf.o" >> %t/mri.script
-RUN: echo "addlib addlib/addlib.ar" >> %t/mri.script
-RUN: echo "addmod delete.o" >> %t/mri.script
-RUN: echo "delete delete.o" >> %t/mri.script
-RUN: echo "save" >> %t/mri.script
-RUN: echo "end" >> %t/mri.script
-
-RUN: cd %t && llvm-ar -M < mri.script
-RUN: FileCheck -input-file=%t/archive.ar %s
-
-CHECK: !<thin>
-CHECK: elf.o
-CHECK-NEXT: addlib/elf.o/
-CHECK-NOT: delete.o
+# RUN: yaml2obj %S/Inputs/elf.yaml -o %t/elf.o
+# RUN: cp %t/elf.o %t/addlib/elf.o
+# RUN: cp %t/elf.o %t/delete.o
+
+# RUN: llvm-ar rc --thin addlib/thin.ar addlib/elf.o
+# RUN: llvm-ar rc addlib/regular.ar addlib/elf.o
+
+# RUN: llvm-ar -M < thin.script
+# RUN: FileCheck -input-file=archive.ar %s --check-prefixes=THIN --implicit-check-not=delete.o --match-full-lines
+
+# THIN: !<thin>
+# THIN: elf.o/
+# THIN-NEXT: addlib/elf.o/
+
+# RUN: llvm-ar -M < thin-to-regular.script
+# RUN: FileCheck -input-file=regular.ar %s --check-prefixes=REGULAR
+
+# REGULAR: !<arch>
+
+# RUN: not llvm-ar -M < regular-to-thin.script 2>&1 | FileCheck %s --check-prefix=ERROR
+# RUN: not ls thin.ar
+# ERROR: error: script line 2: cannot add a regular archive's contents to a thin archive
+
+#--- thin.script
+createthin archive.ar
+addmod elf.o
+addlib addlib/thin.ar
+addmod delete.o
+delete delete.o
+save
+end
+
+#--- thin-to-regular.script
+create regular.ar
+addlib addlib/thin.ar
+save
+end
+
+#--- regular-to-thin.script
+createthin thin.ar
+addlib addlib/regular.ar
+save
+end

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

auto Command = StringSwitch<MRICommand>(CommandStr.lower())
                        .Case("save", MRICommand::Save)
                        .Case("end", MRICommand::End)
                        .Default(MRICommand::Invalid);
 
     switch (Command) {
     case MRICommand::AddLib: {
       object::Archive &Lib = readLibrary(Rest);
       {
+        if (Thin && !Lib.isThin())
+          fail("cannot add a regular archive's contents to a thin archive");
         Error Err = Error::success();
         for (auto &Member : Lib.children(Err))
           addChildMember(NewMembers, Member, /*FlattenArchive=*/Thin);
         failIfError(std::move(Err));
       }
       break;
     }
     case MRICommand::AddMod: