[Tooling] Handle compilation databases containing commands with double dashes

[jnykiel]

Summary

As of CMake commit https://gitlab.kitware.com/cmake/cmake/-/commit/d993ebd4,
which first appeared in CMake 3.19.x series, in the compile commands for
clang-cl, CMake puts -- before the input file. When operating on such a
database, the InterpolatingCompilationDatabase - specifically, the
TransferableCommand constructor - does not recognize that pattern and so, does
not strip the input, or the double dash when 'transferring' the compile command.
This results in a incorrect compile command - with the double dash and old input
file left in, and the language options and new input file appended after them,
where they're all treated as inputs, including the language version option.

Test files for some tests have names similar enough to be matched to commands
from the database, e.g.:

.../path-mappings.test.tmp/server/bar.cpp

can be matched to:

.../Driver/ToolChains/BareMetal.cpp

etc. When that happens, the tool being tested tries to use the matched, and
incorrectly 'transferred' compile command, and fails, reporting errors similar
to:

error: no such file or directory: '/std:c++14'; did you mean '/std:c++14'? [clang-diagnostic-error]

This happens in at least 4 tests:

Clang Tools :: clang-tidy/checkers/performance-trivially-destructible.cpp
Clangd :: check-fail.test
Clangd :: check.test
Clangd :: path-mappings.test

The fix for TransferableCommand removes the -- and everything after it when
determining the arguments that apply to the new file. -- is inserted in the
'transferred' command if the new file name starts with - and when operating in
clang-cl mode, also /. Additionally, other places in the code known to do
argument adjustment without accounting for the -- and causing the tests to
fail are fixed as well.

[aganea]

clang/lib/Tooling/InterpolatingCompilationDatabase.cpp

181

You can leave out the braces on a one-line statement: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

[kadircet]

thanks for looking into this!

we also had a related report about this in clangd, but didn't have time to dive into it. this is definitely fixing an issue in interpolating compilation database, but unfortunately there are still issues lurking around in clangd and command line adjusters.

• any adjuster inserting command line arguments should insert before -- if exists, and any adjuster stripping flags should not strip anything after a --. these can be found in https://github.com/llvm/llvm-project/tree/main/clang/lib/Tooling/ArgumentsAdjusters.cpp
• for clangd we need to ensure resource-dir and sysroot related flags are inserted before --, if exists in https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/CompileCommands.cpp#L211
• insertion of extra flags should happen after -- in https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/ConfigCompile.cpp#L271
• we should again stop stripping flags after -- in https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/CompileCommands.cpp#L478

do you mind fixing those cases too? if not i can also make some follow-ups, but as-is this isn't really useful to tools, at least without the adjuster fixes i mentioned in the first item(as they are used by almost all clang-based tools), the rest is specific to clangd.

clang/lib/Tooling/InterpolatingCompilationDatabase.cpp

182

isn't everything after double dash treated as inputs? i think this should be a break instead of a continue.

244

i think it actually makes sense to insert a double dash here too, if the filename starts with a / or - they might actually be treated as compile flags rather than filenames.

[jnykiel]

I have addressed some of your comments. The second revision is good enough to get the tests to pass - the injectResourceDir change was necessary for the one clang-tidy test that started failing after leaving the -- in. I don't feel familiar enough with clangd specifically to attempt comprehensive fixes there. The clangd issue report specifically mentions -fsyntax-only and -resource-dir as well - so while not comprehensive, the fix may be good for that too.

[jnykiel]

Fixed logic error in injectResourceDir causing a test failure on the build machine.

[kadircet]

In D98824#2634715, @jnykiel wrote:

I have addressed some of your comments. The second revision is good enough to get the tests to pass - the injectResourceDir change was necessary for the one clang-tidy test that started failing after leaving the -- in. I don't feel familiar enough with clangd specifically to attempt comprehensive fixes there. The clangd issue report specifically mentions -fsyntax-only and -resource-dir as well - so while not comprehensive, the fix may be good for that too.

There are a lot more argument adjusters that look at the compile flags and they don't stop at -- though :/ I don't think it is feasible to special case -- in all of those loops and doesn't happen enough in practice (i.e. we see a filename that starts with -resource-dir or -fsyntax-only etc.) to justify a more complex solution. So as mentioned in the comments let's ignore these for now.

clang/lib/Tooling/ArgumentsAdjusters.cpp

47

we should perform this either in all or none. thinking about this again, there are a lot more things that can go wrong but they happen pretty rarely so let's just drop this change and replace the push_back below with getInsertArgumentAdjuster("-fsyntax-only")(AdjustedArgs, "");

copying over args once more isn't crucial here as this happen only once before parsing a translation unit, which by far dominates the latency.

clang/lib/Tooling/InterpolatingCompilationDatabase.cpp

243

the condition should be a function of the Filename, not the compile flag we are using. as the original command might not contain the -- but the filename might still be starting with a - or / here, right?

So i'd suggest Filename.startswith("-") || (ClangCLMode && Filename.startswith("/"))

clang/lib/Tooling/Tooling.cpp

439

similar to comment above, let's just ignore this.

445

nit:

Args = getInsertArgumentAdjuster(("-resource-dir=" + CompilerInvocation::GetResourcesPath(Argv0, MainAddr)).c_str())(Args, "");

There's an overload of getInsertArgumentAdjuster that takes in a const char*

[jnykiel]

I've addressed your comments (I agree with them, essentially). After implementing the insertion of '--' for problematic file names, I had to modify some interpolation unit tests to get them to consistently pass on both Windows and Unix-like systems - I made it possible to match against a relative 'non-native' path in a test, as the absolute paths in this unit test fixture always start with a drive letter on Windows, thus making it impossible to test the '-' cases there, and always start with a '/' on Unix-like systems, making the expected string different when run on Windows or Unix-like.

[kadircet]

thanks, lgtm!

one final nit: getInsertArgumentAdjuster defaults the position to ArgumentInsertPosition::END when there's a single argument to insert. so you can drop those.

[kadircet]

(also please let me know if I should land this for you, preferably with an email address to attribute the commit to)

[jnykiel]

Thanks. About committing/attribution, I'm not sure how it's supposed to work - I've asked a co-worker (@aganea) to commit this, but if he can't do it, please do it instead. I'm Janusz Nykiel, email address janusz.nykiel at ubisoft.com

[kadircet]

SG, will wait for Alexandre to land this then!

clang/lib/Tooling/ArgumentsAdjusters.cpp

return [](const CommandLineArguments &Args, StringRef /*unused*/) {
     constexpr llvm::StringRef OutputCommands[] = {
         // FIXME: Add other options that generate output.
         "-save-temps",
         "--save-temps",
     };
     for (size_t i = 0, e = Args.size(); i < e; ++i) {
       StringRef Arg = Args[i];
       // Skip output commands.
       if (llvm::any_of(OutputCommands, [&Arg](llvm::StringRef OutputCommand) {
             return Arg.startswith(OutputCommand);
           }))
         continue;
 
       if (!Arg.startswith("-fcolor-diagnostics") &&
           !Arg.startswith("-fdiagnostics-color"))
         AdjustedArgs.push_back(Args[i]);
       // If we strip a color option, make sure we strip any preceeding `-Xclang`
       // option as well.
       // FIXME: This should be added to most argument adjusters!
       else if (!AdjustedArgs.empty() && AdjustedArgs.back() == "-Xclang")
         AdjustedArgs.pop_back();
 
       if (Arg == "-fsyntax-only")
         HasSyntaxOnly = true;
     }
     if (!HasSyntaxOnly)
-      AdjustedArgs.push_back("-fsyntax-only");
+      AdjustedArgs =
+          getInsertArgumentAdjuster("-fsyntax-only")(AdjustedArgs, "");
     return AdjustedArgs;
   };
 }
 
 ArgumentsAdjuster getClangStripOutputAdjuster() {
   return [](const CommandLineArguments &Args, StringRef /*unused*/) {
     CommandLineArguments AdjustedArgs;
     for (size_t i = 0, e = Args.size(); i < e; ++i) {

 
 ArgumentsAdjuster getInsertArgumentAdjuster(const CommandLineArguments &Extra,
                                             ArgumentInsertPosition Pos) {
   return [Extra, Pos](const CommandLineArguments &Args, StringRef /*unused*/) {
     CommandLineArguments Return(Args);
 
     CommandLineArguments::iterator I;
     if (Pos == ArgumentInsertPosition::END) {
-      I = Return.end();
+      I = std::find(Return.begin(), Return.end(), "--");
     } else {
       I = Return.begin();
       ++I; // To leave the program name in place
     }
 
     Return.insert(I, Extra.begin(), Extra.end());
     return Return;
   };

clang/lib/Tooling/InterpolatingCompilationDatabase.cpp

for (unsigned Pos = 1; Pos < OldArgs.size();) {
       // Strip input and output files.
       if (Opt.matches(OPT_INPUT) || Opt.matches(OPT_o) ||
           (ClangCLMode && (Opt.matches(OPT__SLASH_Fa) ||
                            Opt.matches(OPT__SLASH_Fe) ||
                            Opt.matches(OPT__SLASH_Fi) ||
                            Opt.matches(OPT__SLASH_Fo))))
         continue;
 
+      // ...including when the inputs are passed after --.
+      if (Opt.matches(OPT__DASH_DASH))
+        break;
+
       // Strip -x, but record the overridden language.
       if (const auto GivenType = tryParseTypeArg(*Arg)) {
         Type = *GivenType;
         continue;
       }
 
       // Strip -std, but record the value.
       if (const auto GivenStd = tryParseStdArg(*Arg)) {
CompileCommand transferTo(StringRef Filename) && {
     }
     // --std flag may only be transferred if the language is the same.
     // We may consider "translating" these, e.g. c++11 -> c11.
     if (Std != LangStandard::lang_unspecified && foldType(TargetType) == Type) {
       Result.CommandLine.emplace_back((
           llvm::Twine(ClangCLMode ? "/std:" : "-std=") +
           LangStandard::getLangStandardForKind(Std).getName()).str());
     }
+    if (Filename.startswith("-") || (ClangCLMode && Filename.startswith("/")))
+      Result.CommandLine.push_back("--");
     Result.CommandLine.push_back(std::string(Filename));
     return Result;
   }
 
 private:
   // Determine whether the given command line is intended for the CL driver.
   static bool checkIsCLMode(ArrayRef<std::string> CmdLine) {
     // First look for --driver-mode.
     for (StringRef S : llvm::reverse(CmdLine)) {

clang/lib/Tooling/Tooling.cpp

 void ClangTool::clearArgumentsAdjusters() {
   ArgsAdjuster = nullptr;
 }
 
 static void injectResourceDir(CommandLineArguments &Args, const char *Argv0,
                               void *MainAddr) {
   // Allow users to override the resource dir.
   for (StringRef Arg : Args)
     if (Arg.startswith("-resource-dir"))
       return;
 
   // If there's no override in place add our resource dir.
-  Args.push_back("-resource-dir=" +
-                 CompilerInvocation::GetResourcesPath(Argv0, MainAddr));
+  Args = getInsertArgumentAdjuster(
+      ("-resource-dir=" + CompilerInvocation::GetResourcesPath(Argv0, MainAddr))
+          .c_str())(Args, "");
 }
 
 int ClangTool::run(ToolAction *Action) {
   // Exists solely for the purpose of lookup of the resource path.
   // This just needs to be some symbol in the binary.
   static int StaticSymbol;
 
   // First insert all absolute paths into the in-memory VFS. These are global

clang/unittests/Tooling/CompilationDatabaseTest.cpp

protected:
 
   MemCDB::EntryMap Entries;
 };
 
 class InterpolateTest : public MemDBTest {
 protected:
   // Look up the command from a relative path, and return it in string form.
   // The input file is not included in the returned command.
-  std::string getCommand(llvm::StringRef F) {
+  std::string getCommand(llvm::StringRef F, bool MakeNative = true) {
     auto Results =
         inferMissingCompileCommands(std::make_unique<MemCDB>(Entries))
-            ->getCompileCommands(path(F));
+            ->getCompileCommands(MakeNative ? path(F) : F);
     if (Results.empty())
       return "none";
     // drop the input file argument, so tests don't have to deal with path().
-    EXPECT_EQ(Results[0].CommandLine.back(), path(F))
+    EXPECT_EQ(Results[0].CommandLine.back(), MakeNative ? path(F) : F)
         << "Last arg should be the file";
     Results[0].CommandLine.pop_back();
     return llvm::join(Results[0].CommandLine, " ");
   }
 
   // Parse the file whose command was used out of the Heuristic string.
   std::string getProxy(llvm::StringRef F) {
     auto Results =

 }
 
 TEST_F(InterpolateTest, Strip) {
   add("dir/foo.cpp", "-o foo.o -Wall");
   // the -o option and the input file are removed, but -Wall is preserved.
   EXPECT_EQ(getCommand("dir/bar.cpp"), "clang -D dir/foo.cpp -Wall");
 }
 
+TEST_F(InterpolateTest, StripDoubleDash) {
+  add("dir/foo.cpp", "-o foo.o -std=c++14 -Wall -- dir/foo.cpp");
+  // input file and output option are removed
+  // -Wall flag isn't
+  // -std option gets re-added as the last argument before the input file
+  // -- is removed as it's not necessary - the new input file doesn't start with
+  // a dash
+  EXPECT_EQ(getCommand("dir/bar.cpp"), "clang -D dir/foo.cpp -Wall -std=c++14");
+}
+
+TEST_F(InterpolateTest, InsertDoubleDash) {
+  add("dir/foo.cpp", "-o foo.o -std=c++14 -Wall");
+  EXPECT_EQ(getCommand("-dir/bar.cpp", false),
+            "clang -D dir/foo.cpp -Wall -std=c++14 --");
+}
+
+TEST_F(InterpolateTest, InsertDoubleDashForClangCL) {
+  add("dir/foo.cpp", "clang-cl", "/std:c++14 /W4");
+  EXPECT_EQ(getCommand("/dir/bar.cpp", false),
+            "clang-cl -D dir/foo.cpp /W4 /std:c++14 --");
+}
+
 TEST_F(InterpolateTest, Case) {
   add("FOO/BAR/BAZ/SHOUT.cc");
   add("foo/bar/baz/quiet.cc");
   // Case mismatches are completely ignored, so we choose the name match.
   EXPECT_EQ(getProxy("foo/bar/baz/shout.C"), "FOO/BAR/BAZ/SHOUT.cc");
 }
 
 TEST_F(InterpolateTest, Aliasing) {
   add("foo.cpp", "-faligned-new");
 
   // The interpolated command should keep the given flag as written, even though
   // the flag is internally represented as an alias.
   EXPECT_EQ(getCommand("foo.hpp"), "clang -D foo.cpp -faligned-new");
 }
 
 TEST_F(InterpolateTest, ClangCL) {
   add("foo.cpp", "clang-cl", "/W4");
 
   // Language flags should be added with CL syntax.
-  EXPECT_EQ(getCommand("foo.h"), "clang-cl -D foo.cpp /W4 /TP");
+  EXPECT_EQ(getCommand("foo.h", false), "clang-cl -D foo.cpp /W4 /TP");
 }
 
 TEST_F(InterpolateTest, DriverModes) {
   add("foo.cpp", "clang-cl", "--driver-mode=gcc");
   add("bar.cpp", "clang", "--driver-mode=cl");
 
   // --driver-mode overrides should be respected.
-  EXPECT_EQ(getCommand("foo.h"), "clang-cl -D foo.cpp --driver-mode=gcc -x c++-header");
-  EXPECT_EQ(getCommand("bar.h"), "clang -D bar.cpp --driver-mode=cl /TP");
+  EXPECT_EQ(getCommand("foo.h"),
+            "clang-cl -D foo.cpp --driver-mode=gcc -x c++-header");
+  EXPECT_EQ(getCommand("bar.h", false),
+            "clang -D bar.cpp --driver-mode=cl /TP");
 }
 
 TEST(TransferCompileCommandTest, Smoke) {
   CompileCommand Cmd;
   Cmd.Filename = "foo.cc";
   Cmd.CommandLine = {"clang", "-Wall", "foo.cc"};
   Cmd.Directory = "dir";
   CompileCommand Transferred = transferCompileCommand(std::move(Cmd), "foo.h");