[PATCH] D26345: Extend small data threshold driver options to PPC target

[Jack Andersen via Phabricator via cfe-commits]

jackoalan updated this revision to Diff 86088.
jackoalan added a comment.
Herald added a subscriber: nemanjai.

Remove already-aliased option matchings; add test case for patch.


https://reviews.llvm.org/D26345

Files:
  lib/Driver/Tools.cpp
  test/Driver/ppc-eabi-small-data.c


Index: test/Driver/ppc-eabi-small-data.c
===
--- /dev/null
+++ test/Driver/ppc-eabi-small-data.c
@@ -0,0 +1,9 @@
+// Check passing PowerPC EABI small-data threshold to the backend.
+
+// RUN: %clang -target powerpc-unknown-unknown-eabi -G 0 %s -### -o %t.o 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-SDATA0 %s
+// RUN: %clang -target powerpc-unknown-unknown-eabi -G 8 %s -### -o %t.o 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-SDATA8 %s
+
+// CHECK-SDATA0: "-mllvm" "-ppc-ssection-threshold=0"
+// CHECK-SDATA8: "-mllvm" "-ppc-ssection-threshold=8"
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -1887,6 +1887,13 @@
 CmdArgs.push_back("-target-abi");
 CmdArgs.push_back(ABIName);
   }
+
+  if (Arg *A = Args.getLastArg(options::OPT_G)) {
+StringRef v = A->getValue();
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back(Args.MakeArgString("-ppc-ssection-threshold=" + v));
+A->claim();
+  }
 }
 
 bool ppc::hasPPCAbiArg(const ArgList , const char *Value) {


Index: test/Driver/ppc-eabi-small-data.c
===
--- /dev/null
+++ test/Driver/ppc-eabi-small-data.c
@@ -0,0 +1,9 @@
+// Check passing PowerPC EABI small-data threshold to the backend.
+
+// RUN: %clang -target powerpc-unknown-unknown-eabi -G 0 %s -### -o %t.o 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-SDATA0 %s
+// RUN: %clang -target powerpc-unknown-unknown-eabi -G 8 %s -### -o %t.o 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-SDATA8 %s
+
+// CHECK-SDATA0: "-mllvm" "-ppc-ssection-threshold=0"
+// CHECK-SDATA8: "-mllvm" "-ppc-ssection-threshold=8"
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -1887,6 +1887,13 @@
 CmdArgs.push_back("-target-abi");
 CmdArgs.push_back(ABIName);
   }
+
+  if (Arg *A = Args.getLastArg(options::OPT_G)) {
+StringRef v = A->getValue();
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back(Args.MakeArgString("-ppc-ssection-threshold=" + v));
+A->claim();
+  }
 }
 
 bool ppc::hasPPCAbiArg(const ArgList , const char *Value) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41534: Don't add empty InstalledDir as a candidate GCC location

[Jack Andersen via Phabricator via cfe-commits]

jackoalan abandoned this revision.
jackoalan added a comment.

I've found that passing `--gcc-toolchain=/usr` results in the exact behavior 
I'm after.

Feel free to reopen if desired.


Repository:
  rC Clang

https://reviews.llvm.org/D41534



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41534: Don't add empty InstalledDir as a candidate GCC location

[Jack Andersen via Phabricator via cfe-commits]

jackoalan created this revision.
jackoalan added a reviewer: cfe-commits.

I maintain a couple build tools based on Clang's Tooling framework. The tools 
are built and used directly out of the CMake binary directory (i.e. not 
installed in a meaningful way).

This non-installed setup causes issues when finding the GCC Toolchain and using 
the resulting system include paths. A normally-installed `clang -v` outputs the 
following:

  clang version 5.0.1 (tags/RELEASE_501/final)
  Target: x86_64-unknown-linux-gnu
  Thread model: posix
  InstalledDir: /usr/bin
  Found candidate GCC installation: 
/usr/bin/../lib/gcc/x86_64-pc-linux-gnu/7.2.1
  Found candidate GCC installation: 
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/7.2.1
  Found candidate GCC installation: /usr/lib/gcc/x86_64-pc-linux-gnu/7.2.1
  Found candidate GCC installation: /usr/lib64/gcc/x86_64-pc-linux-gnu/7.2.1
  Selected GCC installation: /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/7.2.1
  Candidate multilib: .;@m64
  Candidate multilib: 32;@m32
  Selected multilib: .;@m64

But my build-tree tool outputs this:

  clang version 5.0.1 (tags/RELEASE_501/final)
  Target: x86_64-unknown-linux-gnu
  Thread model: posix
  InstalledDir: 
  Found candidate GCC installation: /../lib/gcc/x86_64-pc-linux-gnu/7.2.1
  Found candidate GCC installation: /../lib64/gcc/x86_64-pc-linux-gnu/7.2.1
  Found candidate GCC installation: /usr/lib/gcc/x86_64-pc-linux-gnu/7.2.1
  Found candidate GCC installation: /usr/lib64/gcc/x86_64-pc-linux-gnu/7.2.1
  Selected GCC installation: /../lib64/gcc/x86_64-pc-linux-gnu/7.2.1

InstalledDir is empty, which is fine, but the resulting candidate selection is 
undesirable. The `/usr` rooted candidate strikes me as a much more stable 
option. Granted `/../lib64/gcc/x86_64-pc-linux-gnu/7.2.1` works, but on a 
rather hackish technicality where `/../ == /` and symlink exists at `/lib64`.

This creates problems with dependency files consumed by Ninja (which is unable 
to canonicalize paths that feature a mix of `..` and multi-level symlinks as 
explained in https://reviews.llvm.org/D37954). For example: 
`/../lib64/gcc/x86_64-pc-linux-gnu/7.2.1/../../../../include/c++/7.2.1/cstdlib` 
which ninja "canonicalizes" as `/../include/c++/7.2.1/cstdlib`, but is actually 
`/usr/include/c++/7.2.1/cstdlib` when taking symlinks into account.

If clang were to use the `/usr/lib64/gcc/x86_64-pc-linux-gnu/7.2.1` candidate 
instead, the result would be 
`/usr/lib64/gcc/x86_64-pc-linux-gnu/7.2.1/../../../../include/c++/7.2.1/cstdlib`
 which Ninja can correctly canonicalize to `/usr/include/c++/7.2.1/cstdlib`.

I'm not trying to force clang to produce canonical paths as output; rather 
avoid the obviously problematic `/../` candidate in the first place.


Repository:
  rC Clang

https://reviews.llvm.org/D41534

Files:
  lib/Driver/ToolChains/Gnu.cpp


Index: lib/Driver/ToolChains/Gnu.cpp
===
--- lib/Driver/ToolChains/Gnu.cpp
+++ lib/Driver/ToolChains/Gnu.cpp
@@ -1617,7 +1617,8 @@
 }
 
 // Then look for gcc installed alongside clang.
-Prefixes.push_back(D.InstalledDir + "/..");
+if (!D.InstalledDir.empty())
+  Prefixes.push_back(D.InstalledDir + "/..");
 
 // Then look for distribution supplied gcc installations.
 if (D.SysRoot.empty()) {


Index: lib/Driver/ToolChains/Gnu.cpp
===
--- lib/Driver/ToolChains/Gnu.cpp
+++ lib/Driver/ToolChains/Gnu.cpp
@@ -1617,7 +1617,8 @@
 }
 
 // Then look for gcc installed alongside clang.
-Prefixes.push_back(D.InstalledDir + "/..");
+if (!D.InstalledDir.empty())
+  Prefixes.push_back(D.InstalledDir + "/..");
 
 // Then look for distribution supplied gcc installations.
 if (D.SysRoot.empty()) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115604: [Support] Expand `` as the base directory in configuration files.

[Jack Andersen via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9d37d0ea3485: [Support] Expand `CFGDIR` as the base 
directory in configuration files. (authored by jackoalan).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115604/new/

https://reviews.llvm.org/D115604

Files:
  clang/docs/ReleaseNotes.rst
  clang/docs/UsersManual.rst
  clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -827,7 +827,7 @@
   llvm::BumpPtrAllocator A;
   llvm::StringSaver Saver(A);
   ASSERT_TRUE(llvm::cl::ExpandResponseFiles(
-  Saver, llvm::cl::TokenizeGNUCommandLine, Argv, false, true,
+  Saver, llvm::cl::TokenizeGNUCommandLine, Argv, false, true, false,
   /*CurrentDir=*/StringRef(TestRoot), FS));
   EXPECT_THAT(Argv, testing::Pointwise(
 StringEquality(),
@@ -889,9 +889,9 @@
 #else
   cl::TokenizerCallback Tokenizer = cl::TokenizeGNUCommandLine;
 #endif
-  ASSERT_FALSE(cl::ExpandResponseFiles(Saver, Tokenizer, Argv, false, false,
-   /*CurrentDir=*/llvm::StringRef(TestRoot),
-   FS));
+  ASSERT_FALSE(
+  cl::ExpandResponseFiles(Saver, Tokenizer, Argv, false, false, false,
+  /*CurrentDir=*/llvm::StringRef(TestRoot), FS));
 
   EXPECT_THAT(Argv,
   testing::Pointwise(StringEquality(),
@@ -929,7 +929,7 @@
   BumpPtrAllocator A;
   StringSaver Saver(A);
   ASSERT_FALSE(cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv,
-   false, false,
+   false, false, false,
/*CurrentDir=*/StringRef(TestRoot), FS));
 
   // ASSERT instead of EXPECT to prevent potential out-of-bounds access.
@@ -964,7 +964,7 @@
   BumpPtrAllocator A;
   StringSaver Saver(A);
   ASSERT_TRUE(cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv,
-  false, true,
+  false, true, false,
   /*CurrentDir=*/StringRef(TestRoot), FS));
   EXPECT_THAT(Argv,
   testing::Pointwise(StringEquality(), {"test/test", "-flag"}));
@@ -984,7 +984,7 @@
   BumpPtrAllocator A;
   StringSaver Saver(A);
   ASSERT_TRUE(cl::ExpandResponseFiles(Saver, cl::TokenizeWindowsCommandLine,
-  Argv, true, true,
+  Argv, true, true, false,
   /*CurrentDir=*/StringRef(TestRoot), FS));
   const char *Expected[] = {"clang", "-Xclang", "-Wno-whatever", nullptr,
 "input.cpp"};
@@ -1038,25 +1038,39 @@
   llvm::SmallVector Argv;
 
   TempDir TestDir("unittest", /*Unique*/ true);
+  TempDir TestSubDir(TestDir.path("subdir"), /*Unique*/ false);
 
-  llvm::SmallString<128> TestCfg;
-  llvm::sys::path::append(TestCfg, TestDir.path(), "foo");
-
+  llvm::SmallString<128> TestCfg = TestDir.path("foo");
   TempFile ConfigFile(TestCfg, "",
   "# Comment\n"
   "-option_1\n"
+  "-option_2=/dir1\n"
+  "-option_3=\n"
+  "-option_4 \n"
+  "-option_5=\n"
+  "-option_6=/dir1,/dir2\n"
   "@subconfig\n"
-  "-option_3=abcd\n"
-  "-option_4=\\\n"
+  "-option_11=abcd\n"
+  "-option_12=\\\n"
   "cdef\n");
 
-  llvm::SmallString<128> TestCfg2;
-  llvm::sys::path::append(TestCfg2, TestDir.path(), "subconfig");
+  llvm::SmallString<128> TestCfg2 = TestDir.path("subconfig");
   TempFile ConfigFile2(TestCfg2, "",
-   "-option_2\n"
+   "-option_7\n"
+   "-option_8=/dir2\n"
+   "@subdir/subfoo\n"
"\n"
"   # comment\n");
 
+  llvm::SmallString<128> TestCfg3 = TestSubDir.path("subfoo");
+  TempFile ConfigFile3(TestCfg3, "",
+   "-option_9=/dir3\n"
+   "@/subfoo2\n");
+
+  llvm::SmallString<128> TestCfg4 = TestSubDir.path("subfoo2");
+  TempFile ConfigFile4(TestCfg4, "", "-option_10\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 

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan added inline comments.



Comment at: clang/docs/UsersManual.rst:920
 
+To generate paths relative to the configuration file, the `<@>` token may be
+used. This will expand to the absolute path of the directory containing the

sepavloff wrote:
> 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.
> 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=
-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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115604/new/

https://reviews.llvm.org/D115604

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan added a comment.

bump, review requested


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115604/new/

https://reviews.llvm.org/D115604

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan added a comment.

> 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 `` 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 `` 
as a singular token, but in the future, generic `<(.*)>` tokenization may be 
worth adding.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115604/new/

https://reviews.llvm.org/D115604

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115604: [Support] Expand `` as the base directory in configuration files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan updated this revision to Diff 396576.
jackoalan added a comment.

Update call parameters in `ExpandResponseFilesDatabase::expand`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115604/new/

https://reviews.llvm.org/D115604

Files:
  clang/docs/ReleaseNotes.rst
  clang/docs/UsersManual.rst
  clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -827,7 +827,7 @@
   llvm::BumpPtrAllocator A;
   llvm::StringSaver Saver(A);
   ASSERT_TRUE(llvm::cl::ExpandResponseFiles(
-  Saver, llvm::cl::TokenizeGNUCommandLine, Argv, false, true,
+  Saver, llvm::cl::TokenizeGNUCommandLine, Argv, false, true, false,
   /*CurrentDir=*/StringRef(TestRoot), FS));
   EXPECT_THAT(Argv, testing::Pointwise(
 StringEquality(),
@@ -889,9 +889,9 @@
 #else
   cl::TokenizerCallback Tokenizer = cl::TokenizeGNUCommandLine;
 #endif
-  ASSERT_FALSE(cl::ExpandResponseFiles(Saver, Tokenizer, Argv, false, false,
-   /*CurrentDir=*/llvm::StringRef(TestRoot),
-   FS));
+  ASSERT_FALSE(
+  cl::ExpandResponseFiles(Saver, Tokenizer, Argv, false, false, false,
+  /*CurrentDir=*/llvm::StringRef(TestRoot), FS));
 
   EXPECT_THAT(Argv,
   testing::Pointwise(StringEquality(),
@@ -929,7 +929,7 @@
   BumpPtrAllocator A;
   StringSaver Saver(A);
   ASSERT_FALSE(cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv,
-   false, false,
+   false, false, false,
/*CurrentDir=*/StringRef(TestRoot), FS));
 
   // ASSERT instead of EXPECT to prevent potential out-of-bounds access.
@@ -964,7 +964,7 @@
   BumpPtrAllocator A;
   StringSaver Saver(A);
   ASSERT_TRUE(cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv,
-  false, true,
+  false, true, false,
   /*CurrentDir=*/StringRef(TestRoot), FS));
   EXPECT_THAT(Argv,
   testing::Pointwise(StringEquality(), {"test/test", "-flag"}));
@@ -984,7 +984,7 @@
   BumpPtrAllocator A;
   StringSaver Saver(A);
   ASSERT_TRUE(cl::ExpandResponseFiles(Saver, cl::TokenizeWindowsCommandLine,
-  Argv, true, true,
+  Argv, true, true, false,
   /*CurrentDir=*/StringRef(TestRoot), FS));
   const char *Expected[] = {"clang", "-Xclang", "-Wno-whatever", nullptr,
 "input.cpp"};
@@ -1038,25 +1038,34 @@
   llvm::SmallVector Argv;
 
   TempDir TestDir("unittest", /*Unique*/ true);
+  TempDir TestSubDir(TestDir.path("subdir"), /*Unique*/ false);
 
-  llvm::SmallString<128> TestCfg;
-  llvm::sys::path::append(TestCfg, TestDir.path(), "foo");
-
+  llvm::SmallString<128> 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_7=abcd\n"
+  "-option_8=\\\n"
   "cdef\n");
 
-  llvm::SmallString<128> TestCfg2;
-  llvm::sys::path::append(TestCfg2, TestDir.path(), "subconfig");
+  llvm::SmallString<128> TestCfg2 = TestDir.path("subconfig");
   TempFile ConfigFile2(TestCfg2, "",
-   "-option_2\n"
+   "-option_3\n"
+   "-option_4=/dir2\n"
+   "@subdir/subfoo\n"
"\n"
"   # comment\n");
 
+  llvm::SmallString<128> TestCfg3 = TestSubDir.path("subfoo");
+  TempFile ConfigFile3(TestCfg3, "",
+   "-option_5=/dir3\n"
+   "@/subfoo2\n");
+
+  llvm::SmallString<128> TestCfg4 = TestSubDir.path("subfoo2");
+  TempFile ConfigFile4(TestCfg4, "", "-option_6=qwerty\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
@@ -1071,11 +1080,18 @@
   bool Result = llvm::cl::readConfigFile(ConfigFile.path(), Saver, Argv);
 
   EXPECT_TRUE(Result);
-  EXPECT_EQ(Argv.size(), 4U);
+  EXPECT_EQ(Argv.size(), 8U);
   EXPECT_STREQ(Argv[0], "-option_1");
-  EXPECT_STREQ(Argv[1], 

[PATCH] D115604: [Support] Expand `` as the base directory in response files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan updated this revision to Diff 396410.
jackoalan added a comment.

Rebase, use the slightly more intuitive `` token to expand base paths.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115604/new/

https://reviews.llvm.org/D115604

Files:
  clang/docs/ReleaseNotes.rst
  clang/docs/UsersManual.rst
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -1038,25 +1038,34 @@
   llvm::SmallVector Argv;
 
   TempDir TestDir("unittest", /*Unique*/ true);
+  TempDir TestSubDir(TestDir.path("subdir"), /*Unique*/ false);
 
-  llvm::SmallString<128> TestCfg;
-  llvm::sys::path::append(TestCfg, TestDir.path(), "foo");
-
+  llvm::SmallString<128> 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_7=abcd\n"
+  "-option_8=\\\n"
   "cdef\n");
 
-  llvm::SmallString<128> TestCfg2;
-  llvm::sys::path::append(TestCfg2, TestDir.path(), "subconfig");
+  llvm::SmallString<128> TestCfg2 = TestDir.path("subconfig");
   TempFile ConfigFile2(TestCfg2, "",
-   "-option_2\n"
+   "-option_3\n"
+   "-option_4=/dir2\n"
+   "@subdir/subfoo\n"
"\n"
"   # comment\n");
 
+  llvm::SmallString<128> TestCfg3 = TestSubDir.path("subfoo");
+  TempFile ConfigFile3(TestCfg3, "",
+   "-option_5=/dir3\n"
+   "@/subfoo2\n");
+
+  llvm::SmallString<128> TestCfg4 = TestSubDir.path("subfoo2");
+  TempFile ConfigFile4(TestCfg4, "", "-option_6=qwerty\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
@@ -1071,11 +1080,18 @@
   bool Result = llvm::cl::readConfigFile(ConfigFile.path(), Saver, Argv);
 
   EXPECT_TRUE(Result);
-  EXPECT_EQ(Argv.size(), 4U);
+  EXPECT_EQ(Argv.size(), 8U);
   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=" + TestSubDir.path() + "/dir3").str().c_str());
+  EXPECT_STREQ(Argv[5], "-option_6=qwerty");
+  EXPECT_STREQ(Argv[6], "-option_7=abcd");
+  EXPECT_STREQ(Argv[7], "-option_8=cdef");
 }
 
 TEST(CommandLineTest, PositionalEatArgsError) {
Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -1078,11 +1078,44 @@
   return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' && S[2] == '\xbf');
 }
 
+// Substitute token with the file's base path.
+static void ExpandBasePaths(StringRef BasePath, StringRef Token,
+StringSaver , const char *) {
+  assert(sys::path::is_absolute(BasePath));
+  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)) {
+// Token may appear more than once per arg (e.g. comma-separated linker
+// args). Support 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 token 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 , TokenizerCallback Tokenizer,
-SmallVectorImpl , bool MarkEOLs, bool 

[PATCH] D115604: [Support] Expand `` as the base directory in configuration files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan marked an inline comment as done.
jackoalan added inline comments.



Comment at: llvm/lib/Support/CommandLine.cpp:1099
+else
+  llvm::sys::path::append(ResponseFile, LHS);
+ResponseFile.append(BasePath);

sepavloff wrote:
> What happens if `` is used without trailing path? Such line:
> ```
> --sysroot= -abc
> ```
>  would be processed correctly?
Yes, `if (!Remaining.empty())` handles this case. I've added more test coverage 
for varying expansion contexts.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115604/new/

https://reviews.llvm.org/D115604

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115604: [Support] Expand `` as the base directory in configuration files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan updated this revision to Diff 396667.
jackoalan marked 5 inline comments as done.
jackoalan added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115604/new/

https://reviews.llvm.org/D115604

Files:
  clang/docs/ReleaseNotes.rst
  clang/docs/UsersManual.rst
  clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -827,7 +827,7 @@
   llvm::BumpPtrAllocator A;
   llvm::StringSaver Saver(A);
   ASSERT_TRUE(llvm::cl::ExpandResponseFiles(
-  Saver, llvm::cl::TokenizeGNUCommandLine, Argv, false, true,
+  Saver, llvm::cl::TokenizeGNUCommandLine, Argv, false, true, false,
   /*CurrentDir=*/StringRef(TestRoot), FS));
   EXPECT_THAT(Argv, testing::Pointwise(
 StringEquality(),
@@ -889,9 +889,9 @@
 #else
   cl::TokenizerCallback Tokenizer = cl::TokenizeGNUCommandLine;
 #endif
-  ASSERT_FALSE(cl::ExpandResponseFiles(Saver, Tokenizer, Argv, false, false,
-   /*CurrentDir=*/llvm::StringRef(TestRoot),
-   FS));
+  ASSERT_FALSE(
+  cl::ExpandResponseFiles(Saver, Tokenizer, Argv, false, false, false,
+  /*CurrentDir=*/llvm::StringRef(TestRoot), FS));
 
   EXPECT_THAT(Argv,
   testing::Pointwise(StringEquality(),
@@ -929,7 +929,7 @@
   BumpPtrAllocator A;
   StringSaver Saver(A);
   ASSERT_FALSE(cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv,
-   false, false,
+   false, false, false,
/*CurrentDir=*/StringRef(TestRoot), FS));
 
   // ASSERT instead of EXPECT to prevent potential out-of-bounds access.
@@ -964,7 +964,7 @@
   BumpPtrAllocator A;
   StringSaver Saver(A);
   ASSERT_TRUE(cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv,
-  false, true,
+  false, true, false,
   /*CurrentDir=*/StringRef(TestRoot), FS));
   EXPECT_THAT(Argv,
   testing::Pointwise(StringEquality(), {"test/test", "-flag"}));
@@ -984,7 +984,7 @@
   BumpPtrAllocator A;
   StringSaver Saver(A);
   ASSERT_TRUE(cl::ExpandResponseFiles(Saver, cl::TokenizeWindowsCommandLine,
-  Argv, true, true,
+  Argv, true, true, false,
   /*CurrentDir=*/StringRef(TestRoot), FS));
   const char *Expected[] = {"clang", "-Xclang", "-Wno-whatever", nullptr,
 "input.cpp"};
@@ -1038,25 +1038,38 @@
   llvm::SmallVector Argv;
 
   TempDir TestDir("unittest", /*Unique*/ true);
+  TempDir TestSubDir(TestDir.path("subdir"), /*Unique*/ false);
 
-  llvm::SmallString<128> TestCfg;
-  llvm::sys::path::append(TestCfg, TestDir.path(), "foo");
-
+  llvm::SmallString<128> TestCfg = TestDir.path("foo");
   TempFile ConfigFile(TestCfg, "",
   "# Comment\n"
   "-option_1\n"
+  "-option_2=/dir1\n"
+  "-option_3=\n"
+  "-option_4 \n"
+  "-option_5=\n"
   "@subconfig\n"
-  "-option_3=abcd\n"
-  "-option_4=\\\n"
+  "-option_10=abcd\n"
+  "-option_11=\\\n"
   "cdef\n");
 
-  llvm::SmallString<128> TestCfg2;
-  llvm::sys::path::append(TestCfg2, TestDir.path(), "subconfig");
+  llvm::SmallString<128> TestCfg2 = TestDir.path("subconfig");
   TempFile ConfigFile2(TestCfg2, "",
-   "-option_2\n"
+   "-option_6\n"
+   "-option_7=/dir2\n"
+   "@subdir/subfoo\n"
"\n"
"   # comment\n");
 
+  llvm::SmallString<128> TestCfg3 = TestSubDir.path("subfoo");
+  TempFile ConfigFile3(TestCfg3, "",
+   "-option_8=/dir3\n"
+   "@/subfoo2\n");
+
+  llvm::SmallString<128> TestCfg4 = TestSubDir.path("subfoo2");
+  TempFile ConfigFile4(TestCfg4, "", "-option_9\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
@@ -1071,11 +1084,22 @@
   bool Result = llvm::cl::readConfigFile(ConfigFile.path(), Saver, 

[PATCH] D115604: [Support] Expand `` as the base directory in configuration files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan updated this revision to Diff 396670.
jackoalan added a comment.

Add ReadConfigFile test case for multiple `` in one arg.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115604/new/

https://reviews.llvm.org/D115604

Files:
  clang/docs/ReleaseNotes.rst
  clang/docs/UsersManual.rst
  clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -827,7 +827,7 @@
   llvm::BumpPtrAllocator A;
   llvm::StringSaver Saver(A);
   ASSERT_TRUE(llvm::cl::ExpandResponseFiles(
-  Saver, llvm::cl::TokenizeGNUCommandLine, Argv, false, true,
+  Saver, llvm::cl::TokenizeGNUCommandLine, Argv, false, true, false,
   /*CurrentDir=*/StringRef(TestRoot), FS));
   EXPECT_THAT(Argv, testing::Pointwise(
 StringEquality(),
@@ -889,9 +889,9 @@
 #else
   cl::TokenizerCallback Tokenizer = cl::TokenizeGNUCommandLine;
 #endif
-  ASSERT_FALSE(cl::ExpandResponseFiles(Saver, Tokenizer, Argv, false, false,
-   /*CurrentDir=*/llvm::StringRef(TestRoot),
-   FS));
+  ASSERT_FALSE(
+  cl::ExpandResponseFiles(Saver, Tokenizer, Argv, false, false, false,
+  /*CurrentDir=*/llvm::StringRef(TestRoot), FS));
 
   EXPECT_THAT(Argv,
   testing::Pointwise(StringEquality(),
@@ -929,7 +929,7 @@
   BumpPtrAllocator A;
   StringSaver Saver(A);
   ASSERT_FALSE(cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv,
-   false, false,
+   false, false, false,
/*CurrentDir=*/StringRef(TestRoot), FS));
 
   // ASSERT instead of EXPECT to prevent potential out-of-bounds access.
@@ -964,7 +964,7 @@
   BumpPtrAllocator A;
   StringSaver Saver(A);
   ASSERT_TRUE(cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv,
-  false, true,
+  false, true, false,
   /*CurrentDir=*/StringRef(TestRoot), FS));
   EXPECT_THAT(Argv,
   testing::Pointwise(StringEquality(), {"test/test", "-flag"}));
@@ -984,7 +984,7 @@
   BumpPtrAllocator A;
   StringSaver Saver(A);
   ASSERT_TRUE(cl::ExpandResponseFiles(Saver, cl::TokenizeWindowsCommandLine,
-  Argv, true, true,
+  Argv, true, true, false,
   /*CurrentDir=*/StringRef(TestRoot), FS));
   const char *Expected[] = {"clang", "-Xclang", "-Wno-whatever", nullptr,
 "input.cpp"};
@@ -1038,25 +1038,39 @@
   llvm::SmallVector Argv;
 
   TempDir TestDir("unittest", /*Unique*/ true);
+  TempDir TestSubDir(TestDir.path("subdir"), /*Unique*/ false);
 
-  llvm::SmallString<128> TestCfg;
-  llvm::sys::path::append(TestCfg, TestDir.path(), "foo");
-
+  llvm::SmallString<128> TestCfg = TestDir.path("foo");
   TempFile ConfigFile(TestCfg, "",
   "# Comment\n"
   "-option_1\n"
+  "-option_2=/dir1\n"
+  "-option_3=\n"
+  "-option_4 \n"
+  "-option_5=\n"
+  "-option_6=/dir1,/dir2\n"
   "@subconfig\n"
-  "-option_3=abcd\n"
-  "-option_4=\\\n"
+  "-option_11=abcd\n"
+  "-option_12=\\\n"
   "cdef\n");
 
-  llvm::SmallString<128> TestCfg2;
-  llvm::sys::path::append(TestCfg2, TestDir.path(), "subconfig");
+  llvm::SmallString<128> TestCfg2 = TestDir.path("subconfig");
   TempFile ConfigFile2(TestCfg2, "",
-   "-option_2\n"
+   "-option_7\n"
+   "-option_8=/dir2\n"
+   "@subdir/subfoo\n"
"\n"
"   # comment\n");
 
+  llvm::SmallString<128> TestCfg3 = TestSubDir.path("subfoo");
+  TempFile ConfigFile3(TestCfg3, "",
+   "-option_9=/dir3\n"
+   "@/subfoo2\n");
+
+  llvm::SmallString<128> TestCfg4 = TestSubDir.path("subfoo2");
+  TempFile ConfigFile4(TestCfg4, "", "-option_10\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
@@ -1071,11 +1085,26 @@
   bool Result = llvm::cl::readConfigFile(ConfigFile.path(), Saver, Argv);
 
   

[PATCH] D115604: [Support] Expand `` as the base directory in configuration files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan added a comment.

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,,foo.o

This will cause a trailing `/` to be inserted after ``. However, since 
`` 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115604/new/

https://reviews.llvm.org/D115604

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan updated this revision to Diff 396055.
jackoalan added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115604/new/

https://reviews.llvm.org/D115604

Files:
  clang/docs/ReleaseNotes.rst
  clang/docs/UsersManual.rst
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -1038,25 +1038,34 @@
   llvm::SmallVector Argv;
 
   TempDir TestDir("unittest", /*Unique*/ true);
+  TempDir TestSubDir(TestDir.path("subdir"), /*Unique*/ false);
 
-  llvm::SmallString<128> TestCfg;
-  llvm::sys::path::append(TestCfg, TestDir.path(), "foo");
-
+  llvm::SmallString<128> 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_7=abcd\n"
+  "-option_8=\\\n"
   "cdef\n");
 
-  llvm::SmallString<128> TestCfg2;
-  llvm::sys::path::append(TestCfg2, TestDir.path(), "subconfig");
+  llvm::SmallString<128> TestCfg2 = TestDir.path("subconfig");
   TempFile ConfigFile2(TestCfg2, "",
-   "-option_2\n"
+   "-option_3\n"
+   "-option_4=/dir2\n"
+   "@subdir/subfoo\n"
"\n"
"   # comment\n");
 
+  llvm::SmallString<128> TestCfg3 = TestSubDir.path("subfoo");
+  TempFile ConfigFile3(TestCfg3, "",
+   "-option_5=/dir3\n"
+   "@/subfoo2\n");
+
+  llvm::SmallString<128> TestCfg4 = TestSubDir.path("subfoo2");
+  TempFile ConfigFile4(TestCfg4, "", "-option_6=qwerty\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
@@ -1071,11 +1080,18 @@
   bool Result = llvm::cl::readConfigFile(ConfigFile.path(), Saver, Argv);
 
   EXPECT_TRUE(Result);
-  EXPECT_EQ(Argv.size(), 4U);
+  EXPECT_EQ(Argv.size(), 8U);
   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=" + TestSubDir.path() + "/dir3").str().c_str());
+  EXPECT_STREQ(Argv[5], "-option_6=qwerty");
+  EXPECT_STREQ(Argv[6], "-option_7=abcd");
+  EXPECT_STREQ(Argv[7], "-option_8=cdef");
 }
 
 TEST(CommandLineTest, PositionalEatArgsError) {
Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -1078,11 +1078,44 @@
   return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' && S[2] == '\xbf');
 }
 
+// Substitute token with the file's base path.
+static void ExpandBasePaths(StringRef BasePath, StringRef Token,
+StringSaver , const char *) {
+  assert(sys::path::is_absolute(BasePath));
+  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)) {
+// Token may appear more than once per arg (e.g. comma-separated linker
+// args). Support 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 token 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 

[PATCH] D115604: [Support] Expand `` as the base directory in configuration files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan updated this revision to Diff 396561.
jackoalan retitled this revision from "[Support] Expand `` as the base 
directory in response files." to "[Support] Expand `` as the base 
directory in configuration files.".
jackoalan added a comment.

Make `` constant at point of expansion. Use bool parameter to activate 
`` expansion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115604/new/

https://reviews.llvm.org/D115604

Files:
  clang/docs/ReleaseNotes.rst
  clang/docs/UsersManual.rst
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -827,7 +827,7 @@
   llvm::BumpPtrAllocator A;
   llvm::StringSaver Saver(A);
   ASSERT_TRUE(llvm::cl::ExpandResponseFiles(
-  Saver, llvm::cl::TokenizeGNUCommandLine, Argv, false, true,
+  Saver, llvm::cl::TokenizeGNUCommandLine, Argv, false, true, false,
   /*CurrentDir=*/StringRef(TestRoot), FS));
   EXPECT_THAT(Argv, testing::Pointwise(
 StringEquality(),
@@ -889,9 +889,9 @@
 #else
   cl::TokenizerCallback Tokenizer = cl::TokenizeGNUCommandLine;
 #endif
-  ASSERT_FALSE(cl::ExpandResponseFiles(Saver, Tokenizer, Argv, false, false,
-   /*CurrentDir=*/llvm::StringRef(TestRoot),
-   FS));
+  ASSERT_FALSE(
+  cl::ExpandResponseFiles(Saver, Tokenizer, Argv, false, false, false,
+  /*CurrentDir=*/llvm::StringRef(TestRoot), FS));
 
   EXPECT_THAT(Argv,
   testing::Pointwise(StringEquality(),
@@ -929,7 +929,7 @@
   BumpPtrAllocator A;
   StringSaver Saver(A);
   ASSERT_FALSE(cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv,
-   false, false,
+   false, false, false,
/*CurrentDir=*/StringRef(TestRoot), FS));
 
   // ASSERT instead of EXPECT to prevent potential out-of-bounds access.
@@ -964,7 +964,7 @@
   BumpPtrAllocator A;
   StringSaver Saver(A);
   ASSERT_TRUE(cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv,
-  false, true,
+  false, true, false,
   /*CurrentDir=*/StringRef(TestRoot), FS));
   EXPECT_THAT(Argv,
   testing::Pointwise(StringEquality(), {"test/test", "-flag"}));
@@ -984,7 +984,7 @@
   BumpPtrAllocator A;
   StringSaver Saver(A);
   ASSERT_TRUE(cl::ExpandResponseFiles(Saver, cl::TokenizeWindowsCommandLine,
-  Argv, true, true,
+  Argv, true, true, false,
   /*CurrentDir=*/StringRef(TestRoot), FS));
   const char *Expected[] = {"clang", "-Xclang", "-Wno-whatever", nullptr,
 "input.cpp"};
@@ -1038,25 +1038,34 @@
   llvm::SmallVector Argv;
 
   TempDir TestDir("unittest", /*Unique*/ true);
+  TempDir TestSubDir(TestDir.path("subdir"), /*Unique*/ false);
 
-  llvm::SmallString<128> TestCfg;
-  llvm::sys::path::append(TestCfg, TestDir.path(), "foo");
-
+  llvm::SmallString<128> 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_7=abcd\n"
+  "-option_8=\\\n"
   "cdef\n");
 
-  llvm::SmallString<128> TestCfg2;
-  llvm::sys::path::append(TestCfg2, TestDir.path(), "subconfig");
+  llvm::SmallString<128> TestCfg2 = TestDir.path("subconfig");
   TempFile ConfigFile2(TestCfg2, "",
-   "-option_2\n"
+   "-option_3\n"
+   "-option_4=/dir2\n"
+   "@subdir/subfoo\n"
"\n"
"   # comment\n");
 
+  llvm::SmallString<128> TestCfg3 = TestSubDir.path("subfoo");
+  TempFile ConfigFile3(TestCfg3, "",
+   "-option_5=/dir3\n"
+   "@/subfoo2\n");
+
+  llvm::SmallString<128> TestCfg4 = TestSubDir.path("subfoo2");
+  TempFile ConfigFile4(TestCfg4, "", "-option_6=qwerty\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
@@ -1071,11 +1080,18 @@
   bool Result = llvm::cl::readConfigFile(ConfigFile.path(), Saver, Argv);
 
   EXPECT_TRUE(Result);
-  

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan updated this revision to Diff 393781.
jackoalan added a comment.

Fix ResponseFiles test for Windows.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115604/new/

https://reviews.llvm.org/D115604

Files:
  clang/docs/UsersManual.rst
  llvm/lib/Support/CommandLine.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -784,9 +784,9 @@
 TEST(CommandLineTest, ResponseFiles) {
   vfs::InMemoryFileSystem FS;
 #ifdef _WIN32
-  const char *TestRoot = "C:\\";
+  StringRef TestRoot = "C:\\";
 #else
-  const char *TestRoot = "/";
+  StringRef TestRoot = "/";
 #endif
   FS.setCurrentWorkingDirectory(TestRoot);
 
@@ -796,8 +796,10 @@
  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";
@@ -807,14 +809,16 @@
   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;
@@ -828,13 +832,21 @@
   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"}));
+  /*CurrentDir=*/TestRoot, FS));
+
+  std::string ExpectedOption24 = ("-option_24=" + TestRoot + "incdir").str();
+  std::string ExpectedOption34 = ("-option_34=" + TestRoot + "incdir").str();
+  std::string ExpectedOption5 = ("-option_5=" + TestRoot).str();
+  std::string ExpectedOption6 = ("-option_6=" + TestRoot + "sub").str();
+
+  EXPECT_THAT(
+  Argv, testing::Pointwise(
+StringEquality(),
+{"test/test", "-flag_1", "-option_1", "-option_2", "-option_21",
+ "-option_22", "-option_23=abcd", ExpectedOption24.c_str(),
+ "-option_3=abcd", "-option_31", "-option_32",
+ "-option_33=abcd", ExpectedOption34.c_str(), "-option_4=efjk",
+ ExpectedOption5.c_str(), ExpectedOption6.c_str(), "-flag_2"}));
 }
 
 TEST(CommandLineTest, RecursiveResponseFiles) {
@@ -1045,15 +1057,17 @@
   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");
 
@@ -1071,11 +1085,15 @@
   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");
-  

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan updated this revision to Diff 393777.
jackoalan added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115604/new/

https://reviews.llvm.org/D115604

Files:
  clang/docs/UsersManual.rst
  llvm/lib/Support/CommandLine.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -796,8 +796,10 @@
  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";
@@ -807,14 +809,16 @@
   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;
@@ -829,12 +833,14 @@
   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(
+StringEquality(),
+{"test/test", "-flag_1", "-option_1", "-option_2", "-option_21",
+ "-option_22", "-option_23=abcd", "-option_24=/incdir",
+ "-option_3=abcd", "-option_31", "-option_32",
+ "-option_33=abcd", "-option_34=/incdir", "-option_4=efjk",
+ "-option_5=/", "-option_6=/sub", "-flag_2"}));
 }
 
 TEST(CommandLineTest, RecursiveResponseFiles) {
@@ -1045,15 +1051,17 @@
   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");
 
@@ -1071,11 +1079,15 @@
   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) {
Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan updated this revision to Diff 393785.
jackoalan added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115604/new/

https://reviews.llvm.org/D115604

Files:
  clang/docs/UsersManual.rst
  llvm/lib/Support/CommandLine.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -784,9 +784,9 @@
 TEST(CommandLineTest, ResponseFiles) {
   vfs::InMemoryFileSystem FS;
 #ifdef _WIN32
-  const char *TestRoot = "C:\\";
+  StringRef TestRoot = "C:\\";
 #else
-  const char *TestRoot = "/";
+  StringRef TestRoot = "/";
 #endif
   FS.setCurrentWorkingDirectory(TestRoot);
 
@@ -796,8 +796,11 @@
  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"
+  "-option_7=<@>/sub1,<@>/sub2\n"));
 
   // Directory for included file.
   llvm::StringRef IncDir = "incdir";
@@ -807,14 +810,17 @@
   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"
+"-option_25=<@>/sub1,<@>/sub2\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;
@@ -828,13 +834,27 @@
   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"}));
+  /*CurrentDir=*/TestRoot, FS));
+
+  std::string ExpectedOption24 = ("-option_24=" + TestRoot + "incdir").str();
+  std::string ExpectedOption25 =
+  ("-option_25=" + TestRoot + "incdir/sub1," + TestRoot + "incdir/sub2")
+  .str();
+  std::string ExpectedOption34 = ("-option_34=" + TestRoot + "incdir").str();
+  std::string ExpectedOption5 = ("-option_5=" + TestRoot).str();
+  std::string ExpectedOption6 = ("-option_6=" + TestRoot + "sub").str();
+  std::string ExpectedOption7 =
+  ("-option_7=" + TestRoot + "sub1," + TestRoot + "sub2").str();
+
+  EXPECT_THAT(
+  Argv, testing::Pointwise(
+StringEquality(),
+{"test/test", "-flag_1", "-option_1", "-option_2", "-option_21",
+ "-option_22", "-option_23=abcd", ExpectedOption24.c_str(),
+ ExpectedOption25.c_str(), "-option_3=abcd", "-option_31",
+ "-option_32", "-option_33=abcd", ExpectedOption34.c_str(),
+ "-option_4=efjk", ExpectedOption5.c_str(),
+ ExpectedOption6.c_str(), ExpectedOption7.c_str(), "-flag_2"}));
 }
 
 TEST(CommandLineTest, RecursiveResponseFiles) {
@@ -1045,15 +1065,17 @@
   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, "",
-

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115604/new/

https://reviews.llvm.org/D115604

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116713: [clangd] Support configuration of inlay hints.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan added a comment.

Assuming origin/maain is a branch typo and is no longer being tracked, would 
you please delete the upstream one in 
https://github.com/llvm/llvm-project/branches?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116713/new/

https://reviews.llvm.org/D116713

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits