Allow use of spaces in Bugpoint ‘--compile-command’ argument

[gbreynoo]

Summary

Bug-Point functionality needs extending due to the patch D29185 by bd1976llvm (Allow llvm's build and test systems to support paths with spaces ). It requires Bugpoint to accept the use of spaces within ‘--compile-command’ tokens.

Details
Bugpoint uses the argument ‘--compile-command’ to pass in a command line argument as a string, the string is tokenized by the ‘lexCommand’ function using spaces as a delimiter. Patch D29185 will cause the unit test compile-custom.ll to fail as spaces are now required within tokens and as a delimiter. This patch allows the use of escape characters as below:

Two consecutive '\' evaluate to a single '\'.
A space after a '\' evaluates to a space that is not interpreted as a delimiter.
Any other instances of the '\' character are removed.

Testing
As I explain below, testing may be difficult:

• This change is not dependant on the D29185 patch
• When using a path with no spaces D29185 introduces no new test failures, using a path with spaces will cause a Bugpoint test failure (compile-custom.ll)
• In the review for D29185, Chandlerc makes a point regarding internal paths still not requiring use of spaces. A system test for the new Bugpoint behaviour would not adhere to this
• Bugpoint has no unit tests, it seems out of the scope of this change to create some now

A solution may be using the build bot described in the review of D29185, one that uses a path with spaces. If this patch behaviour changes the build bot will act as a regression test, with the Bugpoint test failing as before.

Any suggestions would be appreciated.

[MatzeB]

Looks generally good, but I'd like to hear a comment on the string handling:

ToolRunner.cpp

379–381 ↗

(On Diff #88351)

Note sure we gain much by moving those 3 characters into a variables (though admittedly already better than the previous code that was searching around in a 1-character std::string).

391 ↗

(On Diff #88351)

For my understanding: CommandLine is an std::string with the last character being '\0'? (It's a bit confusing as I would either expect an std::string with a len but without terminating character or a const char * with terminating character...)

391–393 ↗

(On Diff #88351)

We omit {} for if/for where possible.

397–399 ↗

(On Diff #88351)

No need to braces.

[gbreynoo]

Fixed code in response to MatzeB comments

[gbreynoo]

Thanks for your input MatzeB, this revision should better fit the coding guidelines

[gbreynoo]

Ping

[MatzeB]

LGTM (no further review necessary for the remaining nitpick)

ToolRunner.cpp

386 ↗

(On Diff #89115)

You may consider factoring out CommandLine[Pos] (as in char C = CommandLine[Pos]; in the first line of the loop).

[MatzeB]

http://llvm.org/PR32137 looks like it was caused by this change. Could you please take a look Owen?

[gbreynoo]

In D29940#693761, @MatzeB wrote:

http://llvm.org/PR32137 looks like it was caused by this change. Could you please take a look Owen?

Thanks for the heads-up MatzeB, I have a fix that can be found in the review https://reviews.llvm.org/D30704. I will be away tomorrow so feel free to commit if it looks good.

llvm/trunk/tools/bugpoint/ToolRunner.cpp

Expected<int> CustomExecutor::ExecuteProgram(
 
   return RunProgramWithTimeout(ExecutionCommand, &ProgramArgs[0], InputFile,
                                OutputFile, OutputFile, Timeout, MemoryLimit);
 }
 
 // Tokenize the CommandLine to the command and the args to allow
 // defining a full command line as the command instead of just the
 // executed program. We cannot just pass the whole string after the command
-// as a single argument because then program sees only a single
+// as a single argument because then the program sees only a single
 // command line argument (with spaces in it: "foo bar" instead
 // of "foo" and "bar").
 //
-// code borrowed from:
-// http://oopweb.com/CPP/Documents/CPPHOWTO/Volume/C++Programming-HOWTO-7.html
+// Spaces are used as a delimiter; however repeated, leading, and trailing
+// whitespace are ignored. Simple escaping is allowed via the '\'
+// character, as seen below:
+//
+// Two consecutive '\' evaluate to a single '\'.
+// A space after a '\' evaluates to a space that is not interpreted as a
+// delimiter.
+// Any other instances of the '\' character are removed.
+//
+// Example:
+// '\\' -> '\'
+// '\ ' -> ' '
+// 'exa\mple' -> 'example'
+//
 static void lexCommand(std::string &Message, const std::string &CommandLine,
                        std::string &CmdPath, std::vector<std::string> &Args) {
 
-  std::string Command = "";
-  std::string delimiters = " ";
-
-  std::string::size_type lastPos = CommandLine.find_first_not_of(delimiters, 0);
-  std::string::size_type pos = CommandLine.find_first_of(delimiters, lastPos);
-
-  while (std::string::npos != pos || std::string::npos != lastPos) {
-    std::string token = CommandLine.substr(lastPos, pos - lastPos);
-    if (Command == "")
-      Command = token;
-    else
-      Args.push_back(token);
-    // Skip delimiters.  Note the "not_of"
-    lastPos = CommandLine.find_first_not_of(delimiters, pos);
-    // Find next "non-delimiter"
-    pos = CommandLine.find_first_of(delimiters, lastPos);
+  std::string Token;
+  std::string Command;
+  bool FoundPath = false;
+
+  // first argument is the PATH.
+  // Skip repeated whitespace, leading whitespace and trailing whitespace.
+  for (std::size_t Pos = 0u; Pos <= CommandLine.size(); ++Pos) {
+    if ('\\' == CommandLine[Pos]) {
+      if (Pos + 1 < CommandLine.size())
+        Token.push_back(CommandLine[++Pos]);
+
+      continue;
+    }
+    if (' ' == CommandLine[Pos] || CommandLine.size() == Pos) {
+      if (Token.empty())
+        continue;
+
+      if (!FoundPath) {
+        Command = Token;
+        FoundPath = true;
+        Token.clear();
+        continue;
+      }
+
+      Args.push_back(Token);
+      Token.clear();
+      continue;
+    }
+    Token.push_back(CommandLine[Pos]);
   }
 
   auto Path = sys::findProgramByName(Command);
   if (!Path) {
-    Message = std::string("Cannot find '") + Command + "' in PATH: " +
-              Path.getError().message() + "\n";
+    Message = std::string("Cannot find '") + Command +
+              "' in PATH: " + Path.getError().message() + "\n";
     return;
   }
   CmdPath = *Path;
 
   Message = "Found command in: " + CmdPath + "\n";
 }
 
 // Custom execution environment create method, takes the execution command