This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by gbreynoo on Feb 14 2017, 6:20 AM.

Details

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.

Diff Detail

Event Timeline

gbreynoo created this revision.Feb 14 2017, 6:20 AM
gbreynoo edited the summary of this revision. (Show Details)
MatzeB edited edge metadata.Feb 16 2017, 1:04 PM

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

ToolRunner.cpp
390–408

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).

402

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...)

402–404

We omit {} for if/for where possible.

408–410

No need to braces.

gbreynoo updated this revision to Diff 89115.Feb 20 2017, 7:02 AM

Fixed code in response to MatzeB comments

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

MatzeB accepted this revision.Feb 28 2017, 11:05 AM

LGTM (no further review necessary for the remaining nitpick)

ToolRunner.cpp
386

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

This revision is now accepted and ready to land.Feb 28 2017, 11:05 AM
This revision was automatically updated to reflect the committed changes.
MatzeB added a comment.Mar 6 2017, 3:14 PM

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

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.