[llvm-ar] Include a line number when failing to parse an MRI script

[gbreynoo]

Summary

Errors that occur when reading an MRI script would be clearer if they included a corresponding line number.

[MaskRay]

I wish we can create a new function for MRI error reporting but as it stands, runMRIScript can call readLibrary, addChildMember, and addMember which can in turn call fail or failIfError, so fail has to be aware of the MRI line number. +@grimar @jhenderson in case they have suggestions.

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

10

A bit inconvenience for you (sorry!): after D67558, the error messages are no longer capitalized, so this needs a rebase.

If I recall correctly, one point raised by @grimar or @jhenderson before is that we should still use # comment markers even if the test contains no code. I hope they can confirm.

[grimar]

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

10

Well, I also do not remember now, but I would be happy to see ## as a comment marker everywhere in tests,
or at least at places where there are no markers.
@jhenderson, what do you think?

[jhenderson]

In D67449#1670266, @MaskRay wrote:

I wish we can create a new function for MRI error reporting but as it stands, runMRIScript can call readLibrary, addChildMember, and addMember which can in turn call fail or failIfError, so fail has to be aware of the MRI line number. +@grimar @jhenderson in case they have suggestions.

The only suggestion I have is to pass in an error handler from the calling code that can be used to generate an Error instance with a message customised to the caller. Default would be to do nothing special of course, but it would allow the MRI parser to pass one that refers back to a handler that appends the line number. That's potentially quite a significant refactor, so might be worth delaying to a subsequent change, but it might also be worth being done before this, so that we don't have to introduce the if statement in fail. What do people think?

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

2

Nit: white space -> whitespace

10

I have no strong preference for # versus ##, as long as we use one in tests which don't necessarily need them, as it makes the comments stand-out from the rest of the test (same reason for using ## in tests that need # for RUN lines etc). If someone else has a preference to use ## everywhere, that's fine with me.

[grimar]

In D67449#1671048, @jhenderson wrote:

In D67449#1670266, @MaskRay wrote:

I wish we can create a new function for MRI error reporting but as it stands, runMRIScript can call readLibrary, addChildMember, and addMember which can in turn call fail or failIfError, so fail has to be aware of the MRI line number. +@grimar @jhenderson in case they have suggestions.

The only suggestion I have is to pass in an error handler from the calling code that can be used to generate an Error instance with a message customised to the caller. Default would be to do nothing special of course, but it would allow the MRI parser to pass one that refers back to a handler that appends the line number. That's potentially quite a significant refactor, so might be worth delaying to a subsequent change, but it might also be worth being done before this, so that we don't have to introduce the if statement in fail. What do people think?

I am not good familar with this tool's code yet, but at first look introducing a custom handler looks reasonable (that would help to remove most of these new variables it seems).

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

1042

Is there any reason to check Saved? Can it be just like the following?

ParsingMRIScript = false;

// Nothing to do if not saved.
if (Saved)
  performOperation(ReplaceOrInsert, &NewMembers);

[grimar]

In D67449#1672179, @grimar wrote:

In D67449#1671048, @jhenderson wrote:

In D67449#1670266, @MaskRay wrote:

I wish we can create a new function for MRI error reporting but as it stands, runMRIScript can call readLibrary, addChildMember, and addMember which can in turn call fail or failIfError, so fail has to be aware of the MRI line number. +@grimar @jhenderson in case they have suggestions.

The only suggestion I have is to pass in an error handler from the calling code that can be used to generate an Error instance with a message customised to the caller. Default would be to do nothing special of course, but it would allow the MRI parser to pass one that refers back to a handler that appends the line number. That's potentially quite a significant refactor, so might be worth delaying to a subsequent change, but it might also be worth being done before this, so that we don't have to introduce the if statement in fail. What do people think?

I am not good familar with this tool's code yet, but at first look introducing a custom handler looks reasonable (that would help to remove most of these new variables it seems).

I am not sure though. The current approach is simple and I do not know/see for what else such handler can be usefull here. So I'd just go with this patch for now probably.

[MaskRay]

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

10

Just to make the action item concrete,

Test different ... -> ## Test different ...
RUN: -> # RUN:

[gbreynoo]

Updated the test in line with suggested changes.

[gbreynoo]

Thanks for all the feedback. Regarding an error handler I think the introduction of an error handler could be left for another time.

[grimar]

LGTM with a nit.

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

123–124

It is not clang-formatted.

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

 
 ## 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
 
-# PARSE: error: could not parse library
+# PARSE: error: script line 2: could not parse library
 
 ## No create command.
-# RUN: echo "ADDLIB %t/f.a"  > %t/nocreate.mri
+# RUN: echo "ADDLIB %t/f.a" > %t/nocreate.mri
 # RUN: echo "SAVE" >> %t/nocreate.mri
 # RUN: not llvm-ar -M < %t/nocreate.mri
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL

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

+## Test different MRI comment formats and whitespace.
+
+# RUN: rm -rf %t && mkdir -p %t
+
+# RUN: echo "create %t/mri.ar" > %t/script1.mri
+# RUN: echo "addlib %t/missing.a" >> %t/script1.mri
+
+# RUN: not llvm-ar -M < %t/script1.mri 2>&1 | FileCheck --check-prefix=SCRIPT1 %s
+# SCRIPT1: error: script line 2: could not open library
+
+# RUN: echo "create %t/mri.ar" > %t/script2.mri
+# RUN: echo "addlib %t/bad.a" >> %t/script2.mri
+
+# RUN: echo "bad archive" > %t/bad.a
+
+# RUN: not llvm-ar -M < %t/script2.mri 2>&1 | FileCheck --check-prefix=SCRIPT2 %s
+# SCRIPT2: error: script line 2: could not parse library
+
+# RUN: echo "create %t/mri.ar" > %t/script3.mri
+# RUN: echo "create %t/second.ar" >> %t/script3.mri
+
+# RUN: not llvm-ar -M < %t/script3.mri 2>&1 | FileCheck --check-prefix=SCRIPT3 %s
+# SCRIPT3: error: script line 2: editing multiple archives not supported
+
+# RUN: echo "save" > %t/script4.mri
+# RUN: echo "create %t/mri.ar" >> %t/script4.mri
+
+# RUN: not llvm-ar -M < %t/script4.mri 2>&1 | FileCheck --check-prefix=SCRIPT4 %s
+# SCRIPT4: error: script line 2: file already saved
+
+# RUN: echo "create %t/mri.ar" > %t/script5.mri
+# RUN: echo "bad command" >> %t/script5.mri
+
+# RUN: not llvm-ar -M < %t/script5.mri 2>&1 | FileCheck --check-prefix=SCRIPT5 %s
+# SCRIPT5: error: script line 2: unknown command: bad
+
+# RUN: echo "bad command" | not llvm-ar -M 2>&1 | FileCheck --check-prefix=SCRIPT6 %s
+# SCRIPT6: error: script line 1: unknown command: bad
+
+# RUN: not llvm-ar -M rc %t/mri.ar 2>&1 | FileCheck --check-prefix=SCRIPT7 %s
+# SCRIPT7: error: cannot mix -M and other options

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

 
 void printHelpMessage() {
   if (Stem.contains_lower("ranlib"))
     outs() << RanlibHelp;
   else if (Stem.contains_lower("ar"))
     outs() << ArHelp;
 }
 
+static unsigned MRILineNumber;
+static bool ParsingMRIScript;
+
 // Show the error message and exit.
 LLVM_ATTRIBUTE_NORETURN static void fail(Twine Error) {
+  if (ParsingMRIScript) {
+    WithColor::error(errs(), ToolName)
+        << "script line " << MRILineNumber << ": " << Error << "\n";
+  } else {
     WithColor::error(errs(), ToolName) << Error << "\n";
     printHelpMessage();
+  }
+
   exit(1);
 }
 
 static void failIfError(std::error_code EC, Twine Context = "") {
   if (!EC)
     return;
 
   std::string ContextStr = Context.str();

 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());
   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;
     Line = Line.split(';').first;
     Line = Line.split('*').first;
     Line = Line.trim();
     if (Line.empty())
       continue;
     StringRef CommandStr, Rest;
     std::tie(CommandStr, Rest) = Line.split(' ');
case MRICommand::Save:
       Saved = true;
       break;
     case MRICommand::End:
       break;
     case MRICommand::Invalid:
       fail("unknown command: " + CommandStr);
     }
   }
   
+  ParsingMRIScript = false;
+  
   // Nothing to do if not saved.
   if (Saved)
     performOperation(ReplaceOrInsert, &NewMembers);
   exit(0);
 }
 
 static bool handleGenericOption(StringRef arg) {
   if (arg == "h" || arg.startswith("-h") || arg == "--help") {
     printHelpMessage();
     return true;
   }
   if (arg == "-version" || arg == "--version") {