[llvm-ar] Improve MRI script CREATE command handling

[gbreynoo]

Summary

I discovered that when compared to GNU the llvm-ar MRI script parsing of CREATE could lead to some strange behaviour. This fix improves the error message in the case when no archive name is given and will not allow the adding of members until CREATE is called. Along with this change I added more testing of the CREATE command.

[MaskRay]

Consider precommitting the tests to make the change more focused?

llvm/test/tools/llvm-ar/mri-addlib.test

2–3

# RUN: rm -rf %t && split-file %s %t && cd %t

split-file doesn't need mkdir.

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

1066

output archive is not opened?

[MaskRay]

llvm/test/tools/llvm-ar/mri-addlib.test

38

You may use SYMS2-COUNT-2:

llvm/test/tools/llvm-ar/mri-addmod.test

30

-COUNT-2

[jhenderson]

llvm/test/tools/llvm-ar/mri-addlib.test

27

Nit: here and below, missing full stop at end of sentence.

31

I believe you should use one of the %errc substitutions (I think %errc_NOENT if memory serves correctly, but look it up!)

That way, the message is platform independent.

41–42

FILES2-COUNT-2 along the same lines as above, assuming they are in adjacent lines in the output.

llvm/test/tools/llvm-ar/mri-addmod.test

1

Same comments from me apply to this test.

llvm/test/tools/llvm-ar/mri-create.test

11

Nit: missing full stops.

30

Is CREATE -> SAVE -> CREATE supported? If so, do you need test coverage for that too?

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

1066

I prefer "no output archive has been opened"

[gbreynoo]

Fixed comments, added use of errc_NOENT and COUNT-2, added missing coverage of SAVE between calls to CREATE and improved error message.

[gbreynoo]

llvm/test/tools/llvm-ar/mri-addlib.test

31

Thanks, didn't know about that.

38

Thanks, I didn't know about that.

llvm/test/tools/llvm-ar/mri-create.test

114

I'll fix this before I commit

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

1066

I went with "no output archive has been opened" as it seemed clearer.

[jhenderson]

There's a potentially relevant pre-merge failure in mri-create.test on debian (see above), which you should look at before landing this (I suspect it's the missing %errc substitution I highlighted inline).

llvm/test/tools/llvm-ar/mri-create.test

26

Missing %errc substitution usage here?

[gbreynoo]

Updated error message check.

llvm/test/tools/llvm-ar/mri-create.test

26

Thanks, should be fixed now.

[jhenderson]

LGTM, thanks!

llvm/test/tools/llvm-ar/mri-addlib.test

 ## Test the ADDLIB MRI command.
 
-# RUN: rm -rf %t && mkdir -p %t
-# RUN: yaml2obj %s -o %t/f.o
-# RUN: llvm-ar r %t/f.a %t/f.o
+# RUN: rm -rf %t && split-file %s %t
+# RUN: cd %t
+# RUN: yaml2obj %s -o f.o
+# RUN: llvm-ar r f.a f.o
 
 ## Merge contents of archives.
-# RUN: echo "CREATE %t/addlib.a" > %t/addlib.mri
-# RUN: echo "ADDLIB %t/f.a" >> %t/addlib.mri
-# RUN: echo "SAVE" >> %t/addlib.mri
-# RUN: llvm-ar -M < %t/addlib.mri
-# RUN: llvm-nm --print-armap %t/addlib.a | FileCheck --check-prefix=SYMS %s
-# RUN: llvm-ar t %t/addlib.a | FileCheck --check-prefix=FILES %s
+# RUN: llvm-ar -M < add-lib.mri
+# RUN: llvm-nm --print-armap add-lib.a | FileCheck --check-prefix=SYMS %s
+# RUN: llvm-ar t add-lib.a | FileCheck --check-prefix=FILES %s
 
 # SYMS: f in {{.*}}
 # FILES: f.o
 
 ## ADDLIB with non-archive file.
-# RUN: echo "CREATE %t/badlib.a" > %t/badlib.mri
-# RUN: echo "ADDLIB %s" >> %t/badlib.mri
-# RUN: echo "SAVE" >> %t/badlib.mri
-# RUN: not llvm-ar -M < %t/badlib.mri 2>&1 | FileCheck --check-prefix=PARSE %s
-# RUN: not ls %t/badlib.a
+# RUN: not llvm-ar -M < bad-lib.mri 2>&1 | FileCheck --check-prefix=PARSE %s
+# RUN: not ls bad-lib.a
 
 # PARSE: error: script line 2: could not parse library
 
 ## No create command.
-# RUN: echo "ADDLIB %t/f.a" > %t/nocreate.mri
-# RUN: echo "SAVE" >> %t/nocreate.mri
-# RUN: not llvm-ar -M < %t/nocreate.mri
+# RUN: not llvm-ar -M < no-create.mri 2>&1 | FileCheck --check-prefix=NOCREATE %s
 
+# NOCREATE: error: script line 1: no output archive has been opened
+
+## ADDLIB with missing file.
+# RUN: not llvm-ar -M < missing.mri 2>&1 | FileCheck -DMSG=%errc_ENOENT %s --check-prefix=MISSING
+# RUN: not ls missing.a
+
+# MISSING: error: script line 2: could not open library missing-lib.a: [[MSG]]
+
+## ADD same lib twice.
+# RUN: llvm-ar -M < duplicate.mri
+# RUN: llvm-nm --print-armap duplicate.a | FileCheck --check-prefix=SYMS2 %s
+# RUN: llvm-ar t duplicate.a | FileCheck --check-prefix=FILES2 %s
+
+# SYMS2-COUNT-2: f in {{.*}}
+# FILES2-COUNT-2: f.o
+
+#--- f.yaml
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
 Sections:
   - Name: .text
     Type: SHT_PROGBITS
 Symbols:
     - Name:    f
       Binding: STB_GLOBAL
       Section: .text
 ...
+#--- add-lib.mri
+CREATE add-lib.a
+ADDLIB f.a
+SAVE
+
+#--- text.txt
+I AM NOT AN ARCHIVE
+
+#--- bad-lib.mri
+CREATE bad-lib.a
+ADDLIB text.txt
+SAVE
+
+#--- no-create.mri
+ADDLIB f.a
+SAVE
+
+#--- missing.mri
+CREATE missing.a
+ADDLIB missing-lib.a
+SAVE
+
+#--- duplicate.mri
+CREATE duplicate.a
+ADDLIB f.a
+ADDLIB f.a
+SAVE

llvm/test/tools/llvm-ar/mri-addmod.test

 ## Test the ADDMOD MRI command.
 
-# RUN: rm -rf %t && mkdir -p %t
-# RUN: yaml2obj %s -o %t/f.o
+# RUN: rm -rf %t && split-file %s %t
+# RUN: cd %t
+# RUN: yaml2obj %s -o f.o
 
-# RUN: echo "CREATE %t/addmod.a" > %t/addmod.mri
-# RUN: echo "ADDMOD %t/f.o" >> %t/addmod.mri
-# RUN: echo "SAVE" >> %t/addmod.mri
-# RUN: llvm-ar -M < %t/addmod.mri
-# RUN: llvm-nm --print-armap %t/addmod.a | FileCheck %s
+# RUN: llvm-ar -M < add-mod.mri
+# RUN: llvm-nm --print-armap add-mod.a | FileCheck --check-prefix=SYMS %s
+# RUN: llvm-ar t add-mod.a | FileCheck --check-prefix=FILES %s
 
-# CHECK: f in f.o
+# SYMS: f in {{.*}}
+# FILES: f.o
 
+## No create command.
+# RUN: not llvm-ar -M < no-create.mri 2>&1 | FileCheck --check-prefix=NOCREATE %s
+
+# NOCREATE: error: script line 1: no output archive has been opened
+
+## ADDMOD with missing file.
+# RUN: not llvm-ar -M < missing.mri 2>&1 | FileCheck -DMSG=%errc_ENOENT %s --check-prefix=MISSING
+# RUN: not ls missing.a
+
+# MISSING: error: script line 2: missing.o: [[MSG]]
+
+## ADD same file twice.
+# RUN: llvm-ar -M < duplicate.mri
+# RUN: llvm-nm --print-armap duplicate.a | FileCheck --check-prefix=SYMS2 %s
+# RUN: llvm-ar t duplicate.a | FileCheck --check-prefix=FILES2 %s
+
+# SYMS2-COUNT-2: f in {{.*}}
+# FILES2-COUNT-2: f.o
+
+#--- f.yaml
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
 Sections:
   - Name: .text
     Type: SHT_PROGBITS
 Symbols:
     - Name:    f
       Binding: STB_GLOBAL
       Section: .text
 ...
+#--- add-mod.mri
+CREATE add-mod.a
+ADDMOD f.o
+SAVE
+
+#--- no-create.mri
+ADDMOD f.o
+SAVE
+
+#--- missing.mri
+CREATE missing.a
+ADDMOD missing.o
+SAVE
+
+#--- duplicate.mri
+CREATE duplicate.a
+ADDMOD f.o
+ADDMOD f.o
+SAVE

llvm/test/tools/llvm-ar/mri-create.test

+## Test the CREATE MRI command.
+
+# RUN: rm -rf %t && mkdir -p %t/path/ && split-file %s %t
+# RUN: cd %t
+
+# RUN: llvm-ar -M < create.mri
+# RUN: llvm-ar -M < thin.mri
+
+## Test use of CREATE with no archive name.
+# RUN: not llvm-ar -M < none.mri 2>&1 | FileCheck --check-prefix=NONE %s
+# RUN: not llvm-ar -M < thin-none.mri 2>&1 | FileCheck --check-prefix=NONE %s
+# RUN: not llvm-ar -M < none-add.mri 2>&1 | FileCheck --check-prefix=NONE %s
+# RUN: not llvm-ar -M < thin-none-add.mri 2>&1 | FileCheck --check-prefix=NONE %s
+
+# NONE: error: script line 1: missing archive name
+
+## Test use of paths with CREATE.
+# RUN: llvm-ar -M < path.mri
+# RUN: ls path/path.a
+# RUN: llvm-ar -M < thin-path.mri
+# RUN: ls path/thin-path.a
+
+## Test bad paths with CREATE.
+# RUN: not llvm-ar -M < bad.mri 2>&1 | FileCheck --check-prefix=BAD %s
+# RUN: not llvm-ar -M < thin-bad.mri 2>&1 | FileCheck --check-prefix=BAD %s
+# BAD: error: bad/bad.a: no such file or directory
+
+## Test duplicate use of CREATE.
+# RUN: not llvm-ar -M < create2.mri 2>&1 | FileCheck --check-prefix=MULTIPLE %s -DLINE=2
+# RUN: not llvm-ar -M < thin2.mri 2>&1 | FileCheck --check-prefix=MULTIPLE %s -DLINE=2
+# RUN: not llvm-ar -M < mix1.mri 2>&1 | FileCheck --check-prefix=MULTIPLE %s -DLINE=2
+# RUN: not llvm-ar -M < mix2.mri 2>&1 | FileCheck --check-prefix=MULTIPLE %s -DLINE=2
+# RUN: not llvm-ar -M < save.mri 2>&1 | FileCheck --check-prefix=MULTIPLE %s -DLINE=3
+# RUN: not llvm-ar -M < thin-save.mri 2>&1 | FileCheck --check-prefix=MULTIPLE %s -DLINE=3
+
+# MULTIPLE: error: script line [[LINE]]: editing multiple archives not supported
+
+#--- create.mri
+CREATE create.a
+SAVE
+
+#--- thin.mri
+CREATETHIN thin.a
+SAVE thin.mri
+
+#--- none.mri
+CREATE
+SAVE
+
+#--- thin-none.mri
+CREATETHIN
+SAVE
+
+#--- foo.txt
+FOO
+
+#--- none-add.mri
+CREATE
+ADDMOD foo.txt
+SAVE
+
+#--- thin-none-add.mri
+CREATETHIN
+ADDMOD foo.txt
+SAVE
+
+#--- path.mri
+CREATE path/path.a
+SAVE
+
+#--- thin-path.mri
+CREATETHIN path/thin-path.a
+SAVE
+
+#--- bad.mri
+CREATE bad/bad.a
+SAVE
+
+#--- thin-bad.mri
+CREATETHIN bad/bad.a
+SAVE
+
+#--- create2.mri
+CREATE create1.a
+CREATE create2.a
+SAVE
+
+#--- thin2.mri
+CREATETHIN thin1.a
+CREATETHIN thin2.a
+SAVE
+
+#--- mix1.mri
+CREATE mix1.a
+CREATETHIN mixthin1.a
+SAVE
+
+#--- mix2.mri
+CREATETHIN mixthin2.a
+CREATE mix2.a
+SAVE
+
+#--- save.mri
+CREATE create1.a
+SAVE
+CREATE create2.a
+SAVE
+
+#--- thin-save.mri
+CREATETHIN create1.a
+SAVE
+CREATETHIN create2.a
+SAVE
 No newline at end of file

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

auto Command = StringSwitch<MRICommand>(CommandStr.lower())
                        .Case("createthin", MRICommand::CreateThin)
                        .Case("delete", MRICommand::Delete)
                        .Case("save", MRICommand::Save)
                        .Case("end", MRICommand::End)
                        .Default(MRICommand::Invalid);
 
     switch (Command) {
     case MRICommand::AddLib: {
+      if (!Create)
+        fail("no output archive has been opened");
       object::Archive &Lib = readLibrary(Rest);
       {
         Error Err = Error::success();
         for (auto &Member : Lib.children(Err))
           addChildMember(NewMembers, Member, /*FlattenArchive=*/Thin);
         failIfError(std::move(Err));
       }
       break;
     }
     case MRICommand::AddMod:
+      if (!Create)
+        fail("no output archive has been opened");
       addMember(NewMembers, Rest);
       break;
     case MRICommand::CreateThin:
       Thin = true;
       LLVM_FALLTHROUGH;
     case MRICommand::Create:
       Create = true;
       if (!ArchiveName.empty())
         fail("editing multiple archives not supported");
       if (Saved)
         fail("file already saved");
       ArchiveName = std::string(Rest);
+      if (ArchiveName.empty())
+        fail("missing archive name");
       break;
     case MRICommand::Delete: {
       llvm::erase_if(NewMembers, [=](NewArchiveMember &M) {
         return comparePaths(M.MemberName, Rest);
       });
       break;
     }
     case MRICommand::Save: