[Support] Expand `<CFGDIR>` as the base directory in configuration files.

[jackoalan]

Summary

Extends response file expansion to recognize <CFGDIR> and expand to the
current file's directory. This makes it much easier to author clang config
files rooted in portable, potentially not-installed SDK directories.

A typical use case may be something like the following:

# sample_sdk.cfg
--target=sample
-isystem <CFGDIR>/include
-L <CFGDIR>/lib
-T <CFGDIR>/ldscripts/link.ld

[jackoalan]

It should be evaluated if <@> is a sufficiently unique token to not accidentally break existing users' response files. The use of shell operator characters would likely ensure this.

[jackoalan]

Add entry to user manual explaining the <@> token.

[jackoalan]

Fix ResponseFiles test for Windows.

[jackoalan]

Test coverage for more than one <@> per arg.

[kadircet]

IIUC, the new behavior being introduced by this patch is not the ability of having a way to refer to other files in a config/response file relative way, but rather extending that ability from only the options that start with @ to options that contain @ as a sub-string in any place.

If so, why do we need that exactly? What's wrong with specifying the config/response file as:

--target=sample
-isystem
 @include
-L
 @lib
-T 
@ldscripts/link.ld

instead of having them on the flag and the value on the same line? are there any "real" cases in which you need a foo=@something today and that command line option doesn't have a form without the = in between?

[jackoalan]

In D115604#3188392, @kadircet wrote:

IIUC, the new behavior being introduced by this patch is not the ability of having a way to refer to other files in a config/response file relative way, but rather extending that ability from only the options that start with @ to options that contain @ as a sub-string in any place.

@ actually behaves as an "include" operation (i.e. the contents of the relative file are tokenized into more response file args). It will fail if a directory is passed because it is only used by FileSystem::getBufferForFile.

The <@> operation I am proposing does not perform any file load and simply expands to the base path as the function already knows it.

[jackoalan]

bump, review requested

[sepavloff]

clang/docs/UsersManual.rst

920 ↗

(On Diff #393785)

Response and configuration files are different things, so the documentation must more specific what facility is extended.

According to this implementation the response file treatment is extended. During development of configuration files couple of attempts were taken to extend the functionality of response files (see for example https://reviews.llvm.org/D36045). They were rejected as breaking compatibility with gcc. It does not mean that such extending is not possible, but it should be discussed in wider community. There must be serious reason for such extension and significant use case.

As for using this feature for configuration files, it does not look as very helpful. Configuration files are searched for in several places and the first found file is used. In this case specification of various locations relative to the config-file does not look neither safe nor natural. Specific use case also would be helpful to ground the need for such extension.

[jackoalan]

clang/docs/UsersManual.rst

920 ↗

(On Diff #393785)

Specific use case also would be helpful to ground the need for such extension.

An embedded architecture's community members may wish to deploy their own portable (non-installed) platform packages, while minimizing the user burden of configuring the build system of their choice to set up all the search paths. Ideally, just the --config option should be enough to handle everything necessary for full use of the package.

Consider an embedded architecture "foo". It deploys a common directory which contains a shared configuration file and headers:

-foo-common
  -config.cfg
  -include
    -foo.h
    -stdlib.h
    -string.h
    -...

foo-common/config.cfg:

--target=foo
-isystem <@>/include

foo is implemented by a family of boards. Each board SDK is contained in a directory located next to the common one. Each has their own device libraries and some require special feature and machine flags.

Here is the directory layout for a board named "bar":

-foo-bar
  -config.cfg
  -include
    -bar.h
  -lib
    -libbar.a
    -libc.a
    -libm.a
  -ldscripts
    -link.ld

foo-bar/config.cfg:

@../foo-common/config.cfg
-mcpu=<foo-variant-used-in-bar>
-isystem <@>/include
-L <@>/lib
-T <@>/ldscripts/link.ld

Configuration files are very convenient for setting up compiler options in an accessible way (i.e. not having to depend on hard-coded logic in the driver or cmake/makefiles/etc to set up the environment). However, the inability to express config-relative paths is a major burden for setting up search paths in this use case.

[sepavloff]

Thank you for the detailed explanation. Now I see how you are going to use this facility and think it worth implementation. A couple of questions.

1. It it possible to limit the new syntax to config files only? It would avoid concerns of gcc compatibility.

2. The new token is a weird combination of special symbols, it cannot be understood without reading documentation. Is it possible to use more understandable designation, like <CONFIG> or something similar? It would make config files more readable and allow future extensions.

[jackoalan]

1. It it possible to limit the new syntax to config files only? It would avoid concerns of gcc compatibility.

Yes, ultimately this use case only calls for extending config files.

Is it possible to use more understandable designation, like <CONFIG> or something similar?

I agree, keyword directives like this would be more flexible in the long term and the use of shell redirection characters would theoretically protect any general purpose word. For now, I think it is acceptable to consume <CONFIG> as a singular token, but in the future, generic <(.*)> tokenization may be worth adding.

[jackoalan]

Make expansion token a parameter of ExpandResponseFiles and limit use to readConfigFile. Elaborate manual entry further and add entry to release notes.

[jackoalan]

Rebase, use the slightly more intuitive <CFGDIR> token to expand base paths.

[sepavloff]

The name of the patch could be more precise if you change response files to configuration files.

clang/docs/UsersManual.rst

926–927 ↗

(On Diff #396410)

It does not describe the use case clearly. Such macro is useful in the cases when configuration file is kept together with SDK content so that location of various SDK component can be expressed using the file location. Something about that could be provided here.

llvm/include/llvm/Support/CommandLine.h

2117 ↗

(On Diff #396410)

It seems that this parameter can be boolean. It only instructs to call ExpandBasePaths, which knows what to expand.

2126 ↗

(On Diff #396410)

The same about boolean parameter.

llvm/lib/Support/CommandLine.cpp

1082

It seems that the parameter Token is not needed, as it is always <CFGDIR>.

1122

It also could be boolean.

1182

It could be boolean.

[jackoalan]

Make <CFGDIR> constant at point of expansion. Use bool parameter to activate <CFGDIR> expansion.

[jackoalan]

Update call parameters in ExpandResponseFilesDatabase::expand

[sepavloff]

llvm/lib/Support/CommandLine.cpp

1099

What happens if <CFGDIR> is used without trailing path? Such line:

--sysroot=<CFGDIR> -abc

would be processed correctly?

[jackoalan]

Update ReadConfigFile test with additional <CFGDIR> expansion contexts (non-prefixed, non-suffixed, escaped in middle).

[jackoalan]

llvm/lib/Support/CommandLine.cpp

1099

Yes, if (!Remaining.empty()) handles this case. I've added more test coverage for varying expansion contexts.

[jackoalan]

Add ReadConfigFile test case for multiple <CFGDIR> in one arg.

[jackoalan]

Actually, I just thought of a possible limitation of using path-append when suffixing with something that isn't actually a path component. However, I cannot say how critical this limitation is.

-Wl,-rpath,<CFGDIR>,foo.o

This will cause a trailing / to be inserted after <CFGDIR>. However, since <CFGDIR> is guaranteed to be a directory, a trailing slash should not be harmful in most cases.

The primary motivation of using path-append is to tidy cases that would otherwise expand as dir//file. Although most practical filesystems handle paths with empty components gracefully, vfs::InMemoryFileSystem does not.

Weighing these two issues, I still think it is worth using path-append.

[sepavloff]

LGTM

Thanks!

llvm/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');
 }
 
+// Substitute <@> with the response file base path.
+static void ExpandResponseDirTokens(StringRef BasePath, StringSaver &Saver,
+                                    const char *&Arg) {
+  assert(sys::path::is_absolute(BasePath));
+  constexpr StringLiteral Token("<@>");
+  const StringRef ArgString(Arg);
+
+  SmallString<128> ResponseFile;
+  StringRef::size_type StartPos = 0;
+  for (StringRef::size_type TokenPos = ArgString.find(Token);
+       TokenPos != StringRef::npos;
+       TokenPos = ArgString.find(Token, StartPos)) {
+    // Although it typically does not make sense to use <@> more than once per
+    // arg, support it by using path-append on any subsequent appearances.
+    const StringRef LHS = ArgString.substr(StartPos, TokenPos - StartPos);
+    if (ResponseFile.empty())
+      ResponseFile = LHS;
+    else
+      llvm::sys::path::append(ResponseFile, LHS);
+    ResponseFile.append(BasePath);
+    StartPos = TokenPos + Token.size();
+  }
+
+  if (!ResponseFile.empty()) {
+    // Path-append the remaining arg substring if at least one <@> appeared.
+    const StringRef Remaining = ArgString.substr(StartPos);
+    if (!Remaining.empty())
+      llvm::sys::path::append(ResponseFile, Remaining);
+    Arg = Saver.save(ResponseFile.str()).data();
+  }
+}
+
 // FName must be an absolute path.
 static llvm::Error ExpandResponseFile(
     StringRef FName, StringSaver &Saver, TokenizerCallback Tokenizer,
     SmallVectorImpl<const char *> &NewArgv, bool MarkEOLs, bool RelativeNames,
     llvm::vfs::FileSystem &FS) {
   assert(sys::path::is_absolute(FName));
   llvm::ErrorOr<std::unique_ptr<MemoryBuffer>> MemBufOrErr =
       FS.getBufferForFile(FName);
   if (!MemBufOrErr)
     return llvm::errorCodeToError(MemBufOrErr.getError());
   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))
static llvm::Error ExpandResponseFile(
 
   if (!RelativeNames)
     return Error::success();
   llvm::StringRef BasePath = llvm::sys::path::parent_path(FName);
   // 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.
   for (auto &Arg : NewArgv) {
+    if (!Arg)
+      continue;
+
+    // Substitute <@> with the response file base path.
+    ExpandResponseDirTokens(BasePath, Saver, Arg);
+
     // Skip non-rsp file arguments.
-    if (!Arg || Arg[0] != '@')
+    if (Arg[0] != '@')
       continue;
 
     StringRef FileName(Arg + 1);
     // Skip if non-relative.
     if (!llvm::sys::path::is_relative(FileName))
       continue;
 
     SmallString<128> ResponseFile;
     ResponseFile.push_back('@');
     ResponseFile.append(BasePath);
     llvm::sys::path::append(ResponseFile, FileName);
-    Arg = Saver.save(ResponseFile.c_str()).data();
+    Arg = Saver.save(ResponseFile.str()).data();
   }
   return Error::success();
 }
 
 /// 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 RelativeNames,
                              llvm::Optional<llvm::StringRef> CurrentDir,
                              llvm::vfs::FileSystem &FS) {
   bool AllExpanded = true;
   struct ResponseFileRecord {
     std::string File;
     size_t End;
   };
 
   // To detect recursive response files, we maintain a stack of files and the
   // position of the last argument in the file. This position is updated
   // dynamically as we recursively expand files.

llvm/unittests/Support/CommandLineTest.cpp

#endif
   FS.setCurrentWorkingDirectory(TestRoot);
 
   // Create included response file of first level.
   llvm::StringRef IncludedFileName = "resp1";
   FS.addFile(IncludedFileName, 0,
              llvm::MemoryBuffer::getMemBuffer("-option_1 -option_2\n"
                                               "@incdir/resp2\n"
                                               "-option_3=abcd\n"
-                                              "@incdir/resp3\n"
-                                              "-option_4=efjk\n"));
+                                              "@<@>/incdir/resp3\n"
+                                              "-option_4=efjk\n"
+                                              "-option_5=<@>\n"
+                                              "-option_6=<@>/sub\n"));
 
   // Directory for included file.
   llvm::StringRef IncDir = "incdir";
 
   // Create included response file of second level.
   llvm::SmallString<128> IncludedFileName2;
   llvm::sys::path::append(IncludedFileName2, IncDir, "resp2");
   FS.addFile(IncludedFileName2, 0,
              MemoryBuffer::getMemBuffer("-option_21 -option_22\n"
-                                        "-option_23=abcd\n"));
+                                        "-option_23=abcd\n"
+                                        "-option_24=<@>\n"));
 
   // Create second included response file of second level.
   llvm::SmallString<128> IncludedFileName3;
   llvm::sys::path::append(IncludedFileName3, IncDir, "resp3");
   FS.addFile(IncludedFileName3, 0,
              MemoryBuffer::getMemBuffer("-option_31 -option_32\n"
-                                        "-option_33=abcd\n"));
+                                        "-option_33=abcd\n"
+                                        "-option_34=<@>\n"));
 
   // Prepare 'file' with reference to response file.
   SmallString<128> IncRef;
   IncRef.append(1, '@');
   IncRef.append(IncludedFileName);
   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);
   ASSERT_TRUE(llvm::cl::ExpandResponseFiles(
       Saver, llvm::cl::TokenizeGNUCommandLine, Argv, false, true,
       /*CurrentDir=*/StringRef(TestRoot), FS));
-  EXPECT_THAT(Argv, testing::Pointwise(
-                        StringEquality(),
-                        {"test/test", "-flag_1", "-option_1", "-option_2",
-                         "-option_21", "-option_22", "-option_23=abcd",
-                         "-option_3=abcd", "-option_31", "-option_32",
-                         "-option_33=abcd", "-option_4=efjk", "-flag_2"}));
+  EXPECT_THAT(
+      Argv, testing::Pointwise(
+                {"test/test", "-flag_1", "-option_1", "-option_2", "-option_21",
+                 "-option_22", "-option_23=abcd", "-option_24=/incdir",
+                 "-option_33=abcd", "-option_34=/incdir", "-option_4=efjk",
+                 "-option_5=/", "-option_6=/sub", "-flag_2"}));
 }
 
 TEST(CommandLineTest, RecursiveResponseFiles) {
   vfs::InMemoryFileSystem FS;
 #ifdef _WIN32
   const char *TestRoot = "C:\\";
 #else
   const char *TestRoot = "/";
TEST(CommandLineTest, ReadConfigFile) {
   TempDir TestDir("unittest", /*Unique*/ true);
 
   llvm::SmallString<128> TestCfg;
   llvm::sys::path::append(TestCfg, TestDir.path(), "foo");
 
   TempFile ConfigFile(TestCfg, "",
                       "# Comment\n"
                       "-option_1\n"
+                      "-option_2=<@>/dir1\n"
                       "@subconfig\n"
-                      "-option_3=abcd\n"
-                      "-option_4=\\\n"
+                      "-option_5=abcd\n"
+                      "-option_6=\\\n"
                       "cdef\n");
 
   llvm::SmallString<128> TestCfg2;
   llvm::sys::path::append(TestCfg2, TestDir.path(), "subconfig");
   TempFile ConfigFile2(TestCfg2, "",
-                       "-option_2\n"
+                       "-option_3\n"
+                       "-option_4=<@>/dir2\n"
                        "\n"
                        "   # comment\n");
 
   // Make sure the current directory is not the directory where config files
   // resides. In this case the code that expands response files will not find
   // 'subconfig' unless it resolves nested inclusions relative to the including
   // file.
   llvm::SmallString<128> CurrDir;
   std::error_code EC = llvm::sys::fs::current_path(CurrDir);
   EXPECT_TRUE(!EC);
   EXPECT_NE(CurrDir.str(), TestDir.path());
 
   llvm::BumpPtrAllocator A;
   llvm::StringSaver Saver(A);
   bool Result = llvm::cl::readConfigFile(ConfigFile.path(), Saver, Argv);
 
   EXPECT_TRUE(Result);
-  EXPECT_EQ(Argv.size(), 4U);
+  EXPECT_EQ(Argv.size(), 6U);
   EXPECT_STREQ(Argv[0], "-option_1");
-  EXPECT_STREQ(Argv[1], "-option_2");
-  EXPECT_STREQ(Argv[2], "-option_3=abcd");
-  EXPECT_STREQ(Argv[3], "-option_4=cdef");
+  EXPECT_STREQ(Argv[1],
+               ("-option_2=" + TestDir.path() + "/dir1").str().c_str());
+  EXPECT_STREQ(Argv[2], "-option_3");
+  EXPECT_STREQ(Argv[3],
+               ("-option_4=" + TestDir.path() + "/dir2").str().c_str());
+  EXPECT_STREQ(Argv[4], "-option_5=abcd");
+  EXPECT_STREQ(Argv[5], "-option_6=cdef");
 }
 
 TEST(CommandLineTest, PositionalEatArgsError) {
   cl::ResetCommandLineParser();
 
   StackOption<std::string, cl::list<std::string>> PosEatArgs(
       "positional-eat-args", cl::Positional, cl::desc("<arguments>..."),
       cl::ZeroOrMore, cl::PositionalEatsArgs);