[llvm-ar] Allow MRI scripts to be passed via command line argument

[gbreynoo]

Summary

Some of our customers prefer the input of MRI scripts through command line argument rather than through stdin. I've set up this diff to see if there was interest in this functionality, allowing for either method to be used.

For context:

• gnu-ar only allows use of MRI scripts through stdin.
• -M is the only command in gnu-ar or llvm-ar that requires use of stdin.

[ruiu]

Honestly I'd try to avoid adding a new command line option to llvm-ar. Especially, in this case there are several easy workaround as follows:

• instead of passing contents of MRI script as an external file you can use shell redirection
• if shell is not available or for some reason you don't want to use it, you can write a wrapper that passes an MRI script to llvm-ar

Also, adding an incompatible new feature to llvm-ar will hurt the interoperability with other ar commands. Once you add a new feature and depend on it, you can't go back to the other ar commands.

Compared to the above points, the benefit of doing this is not very clear to me. If you have more contexts, could you elaborate?

[gbreynoo]

Rui makes good points that I struggle to argue with. I'll abandon this review as their is no interest.

llvm/test/tools/llvm-ar/mri5.test

-; RUN: not llvm-ar -M t < %s 2>&1 | FileCheck %s
-; CHECK: error: cannot mix -M and other options
+# RUN: echo contents > %t.txt
+
+# RUN: echo create %t.a > %t.mri
+# RUN: echo addmod %t.txt >> %t.mri
+# RUN: echo save >> %t.mri
+
+# RUN: rm %t.a
+# RUN: llvm-ar -M %t.mri
+# RUN: llvm-ar p %t.a| FileCheck %s --check-prefix=CONTENTS
+# CONTENTS: contents
+
+# RUN: not llvm-ar -M %/t.bad 2>&1 | FileCheck %s --check-prefix=BAD-SCRIPT -DPATH=%/t.bad
+# BAD-SCRIPT: error: could not open MRI Script [[PATH]]: no such file or directory
+
+# RUN: not llvm-ar -M t < %s 2>&1 | FileCheck %s --check-prefix=EXTRA-1
+# EXTRA-1: error: could not open MRI Script t: no such file or directory
+
+# RUN: not llvm-ar -M t v< %s 2>&1 | FileCheck %s --check-prefix=EXTRA-2
+# EXTRA-2: error: cannot mix -M and other options
+
+# RUN: not llvm-ar t -M < %s 2>&1 | FileCheck %s --check-prefix=EXTRA-2
+
+# RUN: rm %t.a
+# RUN: echo -M %/t.mri > %t.response
+# RUN: llvm-ar @%t.response
+# RUN: llvm-ar p %t.a| FileCheck %s --check-prefix=CONTENTS
+
+# stdin has lower precedence
+# RUN: rm %t.a
+# RUN: llvm-ar -M %t.mri < %s
+# RUN: llvm-ar p %t.a| FileCheck %s --check-prefix=CONTENTS
+
+# RUN: not llvm-ar < %t.mri 2>&1 | FileCheck %s --check-prefix=NO-M
+# NO-M: error: an archive name must be specified
+
+# RUN: rm %t.a
+# RUN: cp %t.mri "%t copy.mri"
+# RUN: llvm-ar -M "%t copy.mri"
+# RUN: llvm-ar p %t.a| FileCheck %s --check-prefix=CONTENTS
+
+# RUN: rm %t.a
+# RUN: cp %t.mri "%t£.txt"
+# RUN: llvm-ar -M "%t£.txt"
+# RUN: llvm-ar p %t.a| FileCheck %s --check-prefix=CONTENTS

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

handleAllErrors(std::move(E), [&](const llvm::ErrorInfoBase &EIB) {
     if (ContextStr.empty())
       fail(EIB.message());
     fail(Context + ": " + EIB.message());
   });
 }
 
 static SmallVector<const char *, 256> PositionalArgs;
 
-static bool MRI;
-
 namespace {
 enum Format { Default, GNU, BSD, DARWIN, Unknown };
 }
 
 static Format FormatType = Default;
 
 static std::string Options;
 

 // match for extract/delete operations when there are multiple matches. This is
 // 1-indexed. A value of 0 is invalid, and implies 'N' is not used.
 static int CountParam = 0;
 
 // This variable holds the name of the archive file as given on the
 // command line.
 static std::string ArchiveName;
 
+// This variable holds the name of the MRI script if given on the command line.
+// It defaults to "-" for stdin
+static std::string MRIScriptName = "-";
+
 static std::vector<std::unique_ptr<MemoryBuffer>> ArchiveBuffers;
 static std::vector<std::unique_ptr<object::Archive>> Archives;
 
 // This variable holds the list of member files to proecess, as given
 // on the command line.
 static std::vector<StringRef> Members;
 
 // Static buffer to hold StringRefs.

 // Get the archive file name from the command line
 static void getArchive() {
   if (PositionalArgs.empty())
     fail("an archive name must be specified");
   ArchiveName = PositionalArgs[0];
   PositionalArgs.erase(PositionalArgs.begin());
 }
 
+// Get the MRI script file name from the command line
+static void getMRIScript() {
+  if (PositionalArgs.empty())
+    return;
+  MRIScriptName = PositionalArgs[0];
+  PositionalArgs.erase(PositionalArgs.begin());
+}
+
 static object::Archive &readLibrary(const Twine &Library) {
   auto BufOrErr = MemoryBuffer::getFile(Library, -1, false);
   failIfError(BufOrErr.getError(), "could not open library " + Library);
   ArchiveBuffers.push_back(std::move(*BufOrErr));
   auto LibOrErr =
       object::Archive::create(ArchiveBuffers.back()->getMemBufferRef());
   failIfError(errorToErrorCode(LibOrErr.takeError()),
               "could not parse library");
   Archives.push_back(std::move(*LibOrErr));
   return *Archives.back();
 }
 
 static void runMRIScript();
 
 // Parse the command line options as presented and return the operation
 // specified. Process all modifiers and check to make sure that constraints on
 // modifier/operation pairs have not been violated.
 static ArchiveOperation parseCommandLine() {
-  if (MRI) {
-    if (!PositionalArgs.empty() || !Options.empty())
-      fail("cannot mix -M and other options");
-    runMRIScript();
-  }
-
   // Keep track of number of operations. We can only specify one
   // per execution.
   unsigned NumOperations = 0;
 
   // Keep track of the number of positional modifiers (a,b,i). Only
   // one can be specified.
   unsigned NumPositional = 0;
 
   // Keep track of which operation was requested
   ArchiveOperation Operation;
 
   bool MaybeJustCreateSymTab = false;
 
   for (unsigned i = 0; i < Options.size(); ++i) {
     switch (Options[i]) {
+    case 'M':
+      getMRIScript();
+      if (i > 0 || Options.size() > 1 || !PositionalArgs.empty())
+        fail("cannot mix -M and other options");
+      runMRIScript();
+      break;
     case 'd':
       ++NumOperations;
       Operation = Delete;
       break;
     case 'm':
       ++NumOperations;
       Operation = Move;
       break;
static int performOperation(ArchiveOperation Operation,
 
   performOperation(Operation, nullptr, nullptr, NewMembers);
   return 0;
 }
 
 static void runMRIScript() {
   enum class MRICommand { AddLib, AddMod, Create, CreateThin, Delete, Save, End, Invalid };
 
-  ErrorOr<std::unique_ptr<MemoryBuffer>> Buf = MemoryBuffer::getSTDIN();
-  failIfError(Buf.getError());
+  ErrorOr<std::unique_ptr<MemoryBuffer>> Buf =
+      MemoryBuffer::getFileOrSTDIN(MRIScriptName);
+
+  failIfError(
+      Buf.getError(),
+      MRIScriptName == "-" ? "" : "could not open MRI Script " + MRIScriptName);
   const MemoryBuffer &Ref = *Buf.get();
   bool Saved = false;
   std::vector<NewArchiveMember> NewMembers;
   ParsingMRIScript = true;
 
   for (line_iterator I(Ref, /*SkipBlanks*/ false), E; I != E; ++I) {
     ++MRILineNumber;
     StringRef Line = *I;
if (Arg == "--") {
         PositionalArgs.push_back(Argv[i]);
       break;
     }
     if (Arg[0] == '-') {
       if (Arg.startswith("--"))
         Arg = Argv[i] + 2;
       else
         Arg = Argv[i] + 1;
-      if (Arg == "M") {
-        MRI = true;
-      } else if (MatchFlagWithArg("format")) {
+      if (MatchFlagWithArg("format")) {
         FormatType = StringSwitch<Format>(match)
                          .Case("default", Default)
                          .Case("gnu", GNU)
                          .Case("darwin", DARWIN)
                          .Case("bsd", BSD)
                          .Default(Unknown);
         if (FormatType == Unknown)
           fail(std::string("Invalid format ") + match);