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 ↗

(On Diff #72952)

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 ↗

(On Diff #72952)

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 ↗

(On Diff #72952)

Delete TestDir

llvm/trunk/include/llvm/Support/CommandLine.h

 /// remaining arguments only until the next end of line, when in a response
 /// file.
 ///
 /// \param [in] Saver Delegates back to the caller for saving parsed strings.
 /// \param [in] Tokenizer Tokenization strategy. Typically Unix or Windows.
 /// \param [in,out] Argv Command line into which to expand response files.
 /// \param [in] MarkEOLs Mark end of lines and the end of the response file
 /// with nullptrs in the Argv vector.
+/// \param [in] RelativeNames true if names of nested response files must be
+/// resolved relative to including file.
 /// \return true if all @files were expanded successfully or there were none.
 bool ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
                          SmallVectorImpl<const char *> &Argv,
-                         bool MarkEOLs = false);
+                         bool MarkEOLs = false, bool RelativeNames = false);
 
 /// \brief Mark all options not part of this category as cl::ReallyHidden.
 ///
 /// \param Category the category of options to keep displaying
 ///
 /// Some tools (like clang-format) like to be able to hide all options that are
 /// not specific to the tool. This function allows a tool to specify a single
 /// option category to display in the -help output.

llvm/trunk/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) {
+                               bool MarkEOLs, bool RelativeNames) {
   ErrorOr<std::unique_ptr<MemoryBuffer>> MemBufOrErr =
       MemoryBuffer::getFile(FName);
   if (!MemBufOrErr)
     return false;
   MemoryBuffer &MemBuf = *MemBufOrErr.get();
   StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
 
   // If we have a UTF-16 byte order mark, convert to UTF-8 for parsing.
   ArrayRef<char> BufRef(MemBuf.getBufferStart(), MemBuf.getBufferEnd());
   std::string UTF8Buf;
   if (hasUTF16ByteOrderMark(BufRef)) {
     if (!convertUTF16ToUTF8String(BufRef, UTF8Buf))
       return false;
     Str = StringRef(UTF8Buf);
   }
   // If we see UTF-8 BOM sequence at the beginning of a file, we shall remove
   // 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);
 
+  // If names of nested response files should be resolved relative to including
+  // file, replace the included response file names with their full paths
+  // obtained by required resolution.
+  if (RelativeNames)
+    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()).data();
+          }
+        }
+      }
+
   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) {
+                             bool MarkEOLs, bool RelativeNames) {
   unsigned RspFiles = 0;
   bool AllExpanded = true;
 
   // Don't cache Argv.size() because it can change.
   for (unsigned I = 0; I != Argv.size();) {
     const char *Arg = Argv[I];
     // Check if it is an EOL marker
     if (Arg == nullptr) {
       ++I;
       continue;
     }
     if (Arg[0] != '@') {
       ++I;
       continue;
     }
 
     // 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)) {
+                            MarkEOLs, RelativeNames)) {
       // 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);
     Argv.insert(Argv.begin() + I, ExpandedArgv.begin(), ExpandedArgv.end());

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/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, GetRegisteredSubcommands) {
   EXPECT_FALSE(Opt1);
   EXPECT_FALSE(Opt2);
   for (auto *S : cl::getRegisteredSubcommands()) {
     if (*S)
       EXPECT_EQ("sc2", S->getName());
   }
 }
 
+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, false, true);
+  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);
+  llvm::sys::fs::remove(TestDir);
+}
+
 }  // anonymous namespace