Resolve response file names relative to including file

[sepavloff]

Summary

If a response file included by construct @file itself includes a response file
and that file is specified by relative file name, current behavior is to resolve
the name relative to the current working directory. The change adds additional
flag to ExpandResponseFiles that may be used to resolve nested response file
names relative to including file. With the new mode a set of related response
files may be kept together and reference each other with short position
independent names.
The new mode make work with configuration files more convenient.

[rnk]

Can you elaborate on the compatibility concerns with MSVC and libiberty?

[sepavloff]

According to MSDN, nested response files are not allowed, so no compatibility problems appear https://msdn.microsoft.com/en-us/library/3te4xt0y.aspx:

It is not possible to specify the @ option from within a response file. That is, a response file cannot embed another response file.

As for libiberty, it allows nested response files and interprets text right after @ as file name. So files are searched relative to the cwd of compiler. It is not a problem if absolute file paths are used. It also is not a problem if including file is in the cwd. But if included file is specified by relative name and including file is not in cwd, the behavior would change.

It seems that this situation should not occur in practice because it means that response file must be written with particular cwd of compiler in mind, but some risk of compatibility issues is present.

[sepavloff]

Updated patch

Instead of changing resolution of all nested response file names, introduce
new mode of ExpandResponseFiles, in which relative names are resolved in the
new way. Implementation of config files may use the new mode while other
code continue using previous implementation. Such solution must not cause
compatibility issues.

[rnk]

lib/Support/CommandLine.cpp

947

Can we do the relativization logic here rather than doing two complete scans of the response file tokens? Presumably they will be large.

[sepavloff]

lib/Support/CommandLine.cpp

947

Although it may not be obvious initially, there is no full rescan here. Two functions ExpandResponseFiles and ExpandResponseFiles make recursive response file expansion without actual recursion. ExpandResponseFiles operates on command line already split into arguments by OS command processor. It scans only first symbols of arguments looking for '@'. For each response file it calls ExpandResponseFile, which replaces @file by its split content. It may cause shift arguments that follow @file down but all these arguments are unexpanded. Then ExpandResponseFiles continues the scan from the first argument obtained from the file expansion. So, ExpandResponseFiles is called only once for a response file and ExpandResponseFiles does not scan each argument more than once.

[rnk]

lgtm

unittests/Support/CommandLineTest.cpp

540

Delete TestDir

lib/Support/CommandLine.cpp

 }
 
 // It is called byte order marker but the UTF-8 BOM is actually not affected
 // by the host system's endianness.
 static bool hasUTF8ByteOrderMark(ArrayRef<char> S) {
   return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' && S[2] == '\xbf');
 }
 
-static bool ExpandResponseFile(const char *FName, StringSaver &Saver,
+static bool ExpandResponseFile(StringRef FName, StringSaver &Saver,
                                TokenizerCallback Tokenizer,
                                SmallVectorImpl<const char *> &NewArgv,
                                bool MarkEOLs = false) {
   ErrorOr<std::unique_ptr<MemoryBuffer>> MemBufOrErr =
       MemoryBuffer::getFile(FName);
   if (!MemBufOrErr)
     return false;
   MemoryBuffer &MemBuf = *MemBufOrErr.get();
static bool ExpandResponseFile(StringRef FName, StringSaver &Saver,
   // these bytes before parsing.
   // Reference: http://en.wikipedia.org/wiki/UTF-8#Byte_order_mark
   else if (hasUTF8ByteOrderMark(BufRef))
     Str = StringRef(BufRef.data() + 3, BufRef.size() - 3);
 
   // Tokenize the contents into NewArgv.
   Tokenizer(Str, Saver, NewArgv, MarkEOLs);
 
+  // Scan expanded arguments. If any of them is itself an expansion of a
+  // response file and that file was specified by relative name, replace the
+  // response file name with the full path obtained by resolution of the
+  // specified name relative to the path of current file.
+  for (unsigned I = 0; I < NewArgv.size(); ++I)
+    if (NewArgv[I]) {
+      StringRef Arg = NewArgv[I];
+      if (Arg.front() == '@') {
+        StringRef FileName = Arg.drop_front();
+        if (llvm::sys::path::is_relative(FileName)) {
+          SmallString<128> ResponseFile;
+          ResponseFile.append(1, '@');
+          llvm::sys::path::append(ResponseFile,
+                                  llvm::sys::path::parent_path(FName), FileName);
+          NewArgv[I] = Saver.save(ResponseFile.c_str());
+        }
+      }
+    }
+
   return true;
 }
 
 /// \brief Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.
 bool cl::ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
                              SmallVectorImpl<const char *> &Argv,
                              bool MarkEOLs) {
for (unsigned I = 0; I != Argv.size();) {
 
     // If we have too many response files, leave some unexpanded.  This avoids
     // crashing on self-referential response files.
     if (RspFiles++ > 20)
       return false;
 
     // Replace this response file argument with the tokenization of its
     // contents.  Nested response files are expanded in subsequent iterations.
-    // FIXME: If a nested response file uses a relative path, is it relative to
-    // the cwd of the process or the response file?
     SmallVector<const char *, 0> ExpandedArgv;
     if (!ExpandResponseFile(Arg + 1, Saver, Tokenizer, ExpandedArgv,
                             MarkEOLs)) {
       // We couldn't read this file, so we leave it in the argument stream and
       // move on.
       AllExpanded = false;
       ++I;
       continue;
     }
     Argv.erase(Argv.begin() + I);

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/ADT/SmallString.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/StringSaver.h"
 #include "gtest/gtest.h"
+#include <fstream>
 #include <stdlib.h>
 #include <string>
 
 using namespace llvm;
 
 namespace {
 
 class TempEnvVar {
TEST(CommandLineTest, RemoveFromAllSubCommands) {
   cl::ResetAllOptionOccurrences();
   EXPECT_FALSE(cl::ParseCommandLineOptions(2, args0, nullptr, true));
   cl::ResetAllOptionOccurrences();
   EXPECT_FALSE(cl::ParseCommandLineOptions(3, args1, nullptr, true));
   cl::ResetAllOptionOccurrences();
   EXPECT_FALSE(cl::ParseCommandLineOptions(3, args2, nullptr, true));
 }
 
+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;
+  llvm::sys::path::append(IncludedFileName, TestDir, "resp1");
+  std::ofstream IncludedFile(IncludedFileName.c_str());
+  EXPECT_TRUE(IncludedFile.is_open());
+  IncludedFile << "-option_1 -option_2\n"
+                  "@incdir/resp2\n"
+                  "-option_3=abcd\n";
+  IncludedFile.close();
+
+  // Directory for included file.
+  llvm::SmallString<128> IncDir;
+  llvm::sys::path::append(IncDir, TestDir, "incdir");
+  EC = llvm::sys::fs::create_directory(IncDir);
+  EXPECT_TRUE(!EC);
+
+  // Create included response file of second level.
+  llvm::SmallString<128> IncludedFileName2;
+  llvm::sys::path::append(IncludedFileName2, IncDir, "resp2");
+  std::ofstream IncludedFile2(IncludedFileName2.c_str());
+  EXPECT_TRUE(IncludedFile2.is_open());
+  IncludedFile2 << "-option_21 -option_22\n";
+  IncludedFile2 << "-option_23=abcd\n";
+  IncludedFile2.close();
+
+  // Prepare 'file' with reference to response file.
+  SmallString<128> IncRef;
+  IncRef.append(1, '@');
+  IncRef.append(IncludedFileName.c_str());
+  llvm::SmallVector<const char *, 4> Argv =
+                            { "test/test", "-flag_1", IncRef.c_str(), "-flag_2" };
+
+  // Expand response files.
+  llvm::BumpPtrAllocator A;
+  llvm::StringSaver Saver(A);
+  bool Res = llvm::cl::ExpandResponseFiles(
+                                 Saver, llvm::cl::TokenizeGNUCommandLine, Argv);
+  EXPECT_TRUE(Res);
+  EXPECT_EQ(Argv.size(), 9);
+  EXPECT_STREQ(Argv[0], "test/test");
+  EXPECT_STREQ(Argv[1], "-flag_1");
+  EXPECT_STREQ(Argv[2], "-option_1");
+  EXPECT_STREQ(Argv[3], "-option_2");
+  EXPECT_STREQ(Argv[4], "-option_21");
+  EXPECT_STREQ(Argv[5], "-option_22");
+  EXPECT_STREQ(Argv[6], "-option_23=abcd");
+  EXPECT_STREQ(Argv[7], "-option_3=abcd");
+  EXPECT_STREQ(Argv[8], "-flag_2");
+
+  llvm::sys::fs::remove(IncludedFileName2);
+  llvm::sys::fs::remove(IncDir);
+  llvm::sys::fs::remove(IncludedFileName);
+}
+
 }  // anonymous namespace