Fix processing of path names in response files on Windows

[dmikulin]

Summary

On Windows we need to be able to process response files with Windows-style path names which include backslashes.

[zturner]

I remember someone said they could think of some issues with doing this, but I can't remember what those issues where. +rnk and +ruiu

[ruiu]

I don't think this is a right thing to do because creating a Windows executable doesn't necessarily mean that you are doing it on Command Prompt and vice versa. clang and lld support --rsp-quoting={windows,posix} to explicitly tell the driver how to parse response files. Didn't that work for you?

[rnk]

llvm/lib/Support/CommandLine.cpp

1085 ↗

(On Diff #136644)

I would say Triple(sys::getProcessTriple()).isOSWindows()

llvm/unittests/Support/CommandLineTest.cpp

636 ↗

(On Diff #136644)

ditto

644 ↗

(On Diff #136644)

This assumes a writable cwd. I think you want this pattern:

int FileDescriptor;
SmallString<64> TempPath;
ASSERT_NO_ERROR(
    fs::createTemporaryFile("resp-", ".txt", FileDescriptor, TempPath));

[rnk]

In D43988#1024748, @ruiu wrote:

I don't think this is a right thing to do because creating a Windows executable doesn't necessarily mean that you are doing it on Command Prompt and vice versa. clang and lld support --rsp-quoting={windows,posix} to explicitly tell the driver how to parse response files. Didn't that work for you?

LLVM's tools do not have a standard way to indicate the response file quoting style.

[ruiu]

LLVM's tools do not have a standard way to indicate the response file quoting style.

But if there was a problem that we had to fix by adding --rsp-quoting option, that will become a problem with this patch, no?

[Bigcheese]

In D43988#1024762, @ruiu wrote:

LLVM's tools do not have a standard way to indicate the response file quoting style.

But if there was a problem that we had to fix by adding --rsp-quoting option, that will become a problem with this patch, no?

Adding a --rsp-quoting option globally would likely change the same place this does.

[Bigcheese]

In D43988#1024748, @ruiu wrote:

I don't think this is a right thing to do because creating a Windows executable doesn't necessarily mean that you are doing it on Command Prompt and vice versa. clang and lld support --rsp-quoting={windows,posix} to explicitly tell the driver how to parse response files. Didn't that work for you?

But we still have to choose a default. Why is using GNU parsing on Windows the right thing to do?

[rnk]

I think this is the right default for Windows. We can add an --rsp-quoting argument as a follow-up if someone needs it.

[ruiu]

My memory is vague -- what was the issue that we needed to add --rsp-quoting in the first place?

[dmikulin]

Addressed review comments.

[ruiu]

I don't want to block this, and it sounds reasonable to make Windows command line grammar default on Windows. I'll defer someone who is more familiar with this code than me to give a LGTM.

[rnk]

OK, ship it. :)

llvm/trunk/lib/Support/CommandLine.cpp

 #include "llvm-c/Support.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/Triple.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/ManagedStatic.h"
bool CommandLineParser::ParseCommandLineOptions(int argc,
                                                 StringRef Overview,
                                                 raw_ostream *Errs) {
   assert(hasOptions() && "No options specified!");
 
   // Expand response files.
   SmallVector<const char *, 20> newArgv(argv, argv + argc);
   BumpPtrAllocator A;
   StringSaver Saver(A);
-  ExpandResponseFiles(Saver, TokenizeGNUCommandLine, newArgv);
+  ExpandResponseFiles(Saver,
+         Triple(sys::getProcessTriple()).isOSWindows() ?
+         cl::TokenizeWindowsCommandLine : cl::TokenizeGNUCommandLine,
+         newArgv);
   argv = &newArgv[0];
   argc = static_cast<int>(newArgv.size());
 
   // Copy the program name into ProgName, making sure not to overflow it.
   ProgramName = sys::path::filename(StringRef(argv[0]));
 
   ProgramOverview = Overview;
   bool IgnoreErrors = Errs;

llvm/trunk/unittests/Support/CommandLineTest.cpp

 //===- llvm/unittest/Support/CommandLineTest.cpp - CommandLine tests ------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/CommandLine.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/Triple.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
 #include "llvm/Support/StringSaver.h"
 #include "gtest/gtest.h"
 #include <fstream>
 #include <stdlib.h>
TEST(CommandLineTest, GetRegisteredSubcommands) {
   }
 }
 
 TEST(CommandLineTest, ArgumentLimit) {
   std::string args(32 * 4096, 'a');
   EXPECT_FALSE(llvm::sys::commandLineFitsWithinSystemLimits("cl", args.data()));
 }
 
+TEST(CommandLineTest, ResponseFileWindows) {
+  if (!Triple(sys::getProcessTriple()).isOSWindows())
+    return;
+
+  static cl::list<std::string>
+    InputFilenames(cl::Positional, cl::desc("<input files>"), cl::ZeroOrMore);
+  StackOption<bool> TopLevelOpt("top-level", cl::init(false));
+
+  // Create response file.
+  int FileDescriptor;
+  SmallString<64> TempPath;
+  std::error_code EC =
+      llvm::sys::fs::createTemporaryFile("resp-", ".txt", FileDescriptor, TempPath);
+  EXPECT_TRUE(!EC);
+
+  std::ofstream RspFile(TempPath.c_str());
+  EXPECT_TRUE(RspFile.is_open());
+  RspFile << "-top-level\npath\\dir\\file1\npath/dir/file2";
+  RspFile.close();
+
+  llvm::SmallString<128> RspOpt;
+  RspOpt.append(1, '@');
+  RspOpt.append(TempPath.c_str());
+  const char *args[] = {"prog", RspOpt.c_str()};
+  EXPECT_FALSE(TopLevelOpt);
+  EXPECT_TRUE(
+      cl::ParseCommandLineOptions(2, args, StringRef(), &llvm::nulls()));
+  EXPECT_TRUE(TopLevelOpt);
+  EXPECT_TRUE(InputFilenames[0] == "path\\dir\\file1");
+  EXPECT_TRUE(InputFilenames[1] == "path/dir/file2");
+
+  llvm::sys::fs::remove(TempPath.c_str());
+}
+
 TEST(CommandLineTest, ResponseFiles) {
   llvm::SmallString<128> TestDir;
   std::error_code EC =
     llvm::sys::fs::createUniqueDirectory("unittest", TestDir);
   EXPECT_TRUE(!EC);
 
   // Create included response file of first level.
   llvm::SmallString<128> IncludedFileName;