[clang-tools-extra] f489fb3 - [clangd] Fix sysroot flag handling in CommandMangler to prevent duplicates (#75694)

[via cfe-commits]

Author: Kon
Date: 2024-01-12T03:11:38-05:00
New Revision: f489fb3d75aaedf6f056113370a8b3b8659b9f17

URL: https://github.com/llvm/llvm-project/commit/f489fb3d75aaedf6f056113370a8b3b8659b9f17
DIFF: https://github.com/llvm/llvm-project/commit/f489fb3d75aaedf6f056113370a8b3b8659b9f17.diff

LOG: [clangd] Fix sysroot flag handling in CommandMangler to prevent duplicates (#75694)

CommandMangler should guess the sysroot path of the host system and add
that through `-isysroot` flag only when there is no `--sysroot` or
`-isysroot` flag in the original compile command to avoid duplicate
sysroot.

Previously, CommandMangler appropriately avoided adding a guessed
sysroot flag if the original command had an argument in the form of
`--sysroot=<sysroot>`, `--sysroot <sysroot>`, or `-isysroot <sysroot>`.
However, when presented as `-isysroot<sysroot>` (without spaces after
`-isysroot`), CommandMangler mistakenly appended the guessed sysroot
flag, resulting in duplicated sysroot in the final command.

This commit fixes it, ensuring the final command has no duplicate
sysroot flags. Also adds unit tests for this fix.

Added: 
    

Modified: 
    clang-tools-extra/clangd/CompileCommands.cpp
    clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/CompileCommands.cpp b/clang-tools-extra/clangd/CompileCommands.cpp
index f4e8e7e74a3bee..1b4b24d59cb83a 100644
--- a/clang-tools-extra/clangd/CompileCommands.cpp
+++ b/clang-tools-extra/clangd/CompileCommands.cpp
@@ -313,13 +313,15 @@ void CommandMangler::operator()(tooling::CompileCommand &Command,
 
   tooling::addTargetAndModeForProgramName(Cmd, Cmd.front());
 
-  // Check whether the flag exists, either as -flag or -flag=*
-  auto Has = [&](llvm::StringRef Flag) {
-    for (llvm::StringRef Arg : Cmd) {
-      if (Arg.consume_front(Flag) && (Arg.empty() || Arg[0] == '='))
-        return true;
-    }
-    return false;
+  // Check whether the flag exists in the command.
+  auto HasExact = [&](llvm::StringRef Flag) {
+    return llvm::any_of(Cmd, [&](llvm::StringRef Arg) { return Arg == Flag; });
+  };
+
+  // Check whether the flag appears in the command as a prefix.
+  auto HasPrefix = [&](llvm::StringRef Flag) {
+    return llvm::any_of(
+        Cmd, [&](llvm::StringRef Arg) { return Arg.startswith(Flag); });
   };
 
   llvm::erase_if(Cmd, [](llvm::StringRef Elem) {
@@ -327,12 +329,13 @@ void CommandMangler::operator()(tooling::CompileCommand &Command,
   });
 
   std::vector<std::string> ToAppend;
-  if (ResourceDir && !Has("-resource-dir"))
+  if (ResourceDir && !HasExact("-resource-dir") && !HasPrefix("-resource-dir="))
     ToAppend.push_back(("-resource-dir=" + *ResourceDir));
 
   // Don't set `-isysroot` if it is already set or if `--sysroot` is set.
   // `--sysroot` is a superset of the `-isysroot` argument.
-  if (Sysroot && !Has("-isysroot") && !Has("--sysroot")) {
+  if (Sysroot && !HasPrefix("-isysroot") && !HasExact("--sysroot") &&
+      !HasPrefix("--sysroot=")) {
     ToAppend.push_back("-isysroot");
     ToAppend.push_back(*Sysroot);
   }
@@ -343,7 +346,7 @@ void CommandMangler::operator()(tooling::CompileCommand &Command,
   }
 
   if (!Cmd.empty()) {
-    bool FollowSymlink = !Has("-no-canonical-prefixes");
+    bool FollowSymlink = !HasExact("-no-canonical-prefixes");
     Cmd.front() =
         (FollowSymlink ? ResolvedDrivers : ResolvedDriversNoFollow)
             .get(Cmd.front(), [&, this] {

diff  --git a/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp b/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
index 772177b60b5eed..b64dd4acad4c22 100644
--- a/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -370,9 +370,8 @@ TEST(ArgStripperTest, OrderDependent) {
 }
 
 TEST(PrintArgvTest, All) {
-  std::vector<llvm::StringRef> Args = {
-      "one", "two", "thr ee", "f\"o\"ur", "fi\\ve", "$"
-  };
+  std::vector<llvm::StringRef> Args = {"one",      "two",    "thr ee",
+                                       "f\"o\"ur", "fi\\ve", "$"};
   const char *Expected = R"(one two "thr ee" "f\"o\"ur" "fi\\ve" $)";
   EXPECT_EQ(Expected, printArgv(Args));
 }
@@ -450,6 +449,82 @@ TEST(CommandMangler, PathsAsPositional) {
   Mangler(Cmd, "a.cc");
   EXPECT_THAT(Cmd.CommandLine, Contains("foo"));
 }
+
+TEST(CommandMangler, RespectsOriginalResourceDir) {
+  auto Mangler = CommandMangler::forTests();
+  Mangler.ResourceDir = testPath("fake/resources");
+
+  {
+    tooling::CompileCommand Cmd;
+    Cmd.CommandLine = {"clang++", "-resource-dir", testPath("true/resources"),
+                       "foo.cc"};
+    Mangler(Cmd, "foo.cc");
+    EXPECT_THAT(llvm::join(Cmd.CommandLine, " "),
+                HasSubstr("-resource-dir " + testPath("true/resources")));
+    EXPECT_THAT(llvm::join(Cmd.CommandLine, " "),
+                Not(HasSubstr(testPath("fake/resources"))));
+  }
+
+  {
+    tooling::CompileCommand Cmd;
+    Cmd.CommandLine = {"clang++", "-resource-dir=" + testPath("true/resources"),
+                       "foo.cc"};
+    Mangler(Cmd, "foo.cc");
+    EXPECT_THAT(llvm::join(Cmd.CommandLine, " "),
+                HasSubstr("-resource-dir=" + testPath("true/resources")));
+    EXPECT_THAT(llvm::join(Cmd.CommandLine, " "),
+                Not(HasSubstr(testPath("fake/resources"))));
+  }
+}
+
+TEST(CommandMangler, RespectsOriginalSysroot) {
+  auto Mangler = CommandMangler::forTests();
+  Mangler.Sysroot = testPath("fake/sysroot");
+
+  {
+    tooling::CompileCommand Cmd;
+    Cmd.CommandLine = {"clang++", "-isysroot", testPath("true/sysroot"),
+                       "foo.cc"};
+    Mangler(Cmd, "foo.cc");
+    EXPECT_THAT(llvm::join(Cmd.CommandLine, " "),
+                HasSubstr("-isysroot " + testPath("true/sysroot")));
+    EXPECT_THAT(llvm::join(Cmd.CommandLine, " "),
+                Not(HasSubstr(testPath("fake/sysroot"))));
+  }
+
+  {
+    tooling::CompileCommand Cmd;
+    Cmd.CommandLine = {"clang++", "-isysroot" + testPath("true/sysroot"),
+                       "foo.cc"};
+    Mangler(Cmd, "foo.cc");
+    EXPECT_THAT(llvm::join(Cmd.CommandLine, " "),
+                HasSubstr("-isysroot" + testPath("true/sysroot")));
+    EXPECT_THAT(llvm::join(Cmd.CommandLine, " "),
+                Not(HasSubstr(testPath("fake/sysroot"))));
+  }
+
+  {
+    tooling::CompileCommand Cmd;
+    Cmd.CommandLine = {"clang++", "--sysroot", testPath("true/sysroot"),
+                       "foo.cc"};
+    Mangler(Cmd, "foo.cc");
+    EXPECT_THAT(llvm::join(Cmd.CommandLine, " "),
+                HasSubstr("--sysroot " + testPath("true/sysroot")));
+    EXPECT_THAT(llvm::join(Cmd.CommandLine, " "),
+                Not(HasSubstr(testPath("fake/sysroot"))));
+  }
+
+  {
+    tooling::CompileCommand Cmd;
+    Cmd.CommandLine = {"clang++", "--sysroot=" + testPath("true/sysroot"),
+                       "foo.cc"};
+    Mangler(Cmd, "foo.cc");
+    EXPECT_THAT(llvm::join(Cmd.CommandLine, " "),
+                HasSubstr("--sysroot=" + testPath("true/sysroot")));
+    EXPECT_THAT(llvm::join(Cmd.CommandLine, " "),
+                Not(HasSubstr(testPath("fake/sysroot"))));
+  }
+}
 } // namespace
 } // namespace clangd
 } // namespace clang