[clangd] Move standard options adaptor to CommandMangler

[DmitryPolukhin]

Summary

There is a discrepancy between how clangd processes CDB loaded from
JSON file on disk and pushed via LSP. Thus the same CDB pushed via
LSP protocol may not work as expected. Some difference between these two
paths is expected but we still need to insert driver mode and target from
binary name and expand response files.

Part of the diff addTargetAndModeForProgramName came from D138546

Test Plan: check-clang-tools

[DmitryPolukhin]

Fix clang-format sources

[nridge]

Note that D138546 is removing the call to inferTargetAndDriverMode() from GlobalCompilationDatabase.cpp. It adds equivalent logic too CommandMangler which is used for CDBs pushed from LSP as well, so it accomplishes objective of this patch with respect to inferTargetAndDriverMode (but not inferMissingCompileCommands).

[DmitryPolukhin]

In D143436#4120899, @nridge wrote:

Note that D138546 is removing the call to inferTargetAndDriverMode() from GlobalCompilationDatabase.cpp. It adds equivalent logic too CommandMangler which is used for CDBs pushed from LSP as well, so it accomplishes objective of this patch with respect to inferTargetAndDriverMode (but not inferMissingCompileCommands).

@nridge I think D138546 solves the issue only partially and we need both patches applied. inferMissingCompileCommands and expandResponseFiles also needs to be applied to command pushed via LSP. I can rebase on top of D138546 if it will be landed first and update test. Please advise which patch should land first?

[kadircet]

There is a discrepancy between how clangd processes CDB loaded from JSON file on disk and pushed via LSP.

That's actually because we model compile commands pushed via LSP as a "fixed compilation database" rather than a json compilation database (you can check the code in parseFixed, near parseJson).
The reason behind the discrepancy between fixed and json compilation databases mostly arises from the fact that the former is written by people specifically to be used by clang-tooling, whereas the latter is usually provided by build system and doesn't necessarily have the same concerns as clang-tooling hence requires certain modifications.

That being said, the two particular transformations (response files & target/mode inference) seem like outliers when it comes to such transformations. These are handled by clang-driver binary, outside of the driver library, in https://github.com/llvm/llvm-project/blob/main/clang/tools/driver/driver.cpp#L426 and https://github.com/llvm/llvm-project/blob/main/clang/tools/driver/driver.cpp#L534. Therefore all the other tools that wants to make use of clang infra tries to simulate this logic, at different layers with slight differences (at the bare minimum, as the logic isn't shared and people mostly care about the driver, logic in other places just gets out-of-date).
I believe the right thing to do here is putting all that arg preparation logic into driver library itself, so that all code paths that tries to create a compiler invocation can have the same behaviour. Unfortunately this is likely too hard to pull at this stage, due to people relying on almost every implementation detail of these interfaces.
So a middle ground would probably involve, moving that logic inside driver binary to a common library, so that future users can at least directly use it rather than trying to come up with their own interpretation (or trying to chose from existing N patterns) and we'll get rid of the issues that result from logic in this place and its duplicates getting out-of-sync. This way we don't need to migrate all the applications that want to create a compiler invocation to a new API and can only migrate clangd (CommandMangler is I believe the right layer for these purposes. as it handles all "string"-like modifications on the compile flags today).

Does that make sense? Is it something you'd like to propose a patch for? Because as things stand I think we're just making the matter worse here by introducing some new code paths that're trying to emulate the logic in driver today and will get out of sync at some point.

clang-tools-extra/clangd/ClangdLSPServer.cpp

1350 ↗

(On Diff #495431)

it looks like this comparison is no longer valid.

OverlayCDB::getCompileCommand will apply more modifications to the stored flags than just expanding response files and inferring targets. I believe the intent of this interaction was to treat compile flags from the LSP client as-is without any modifications (as I explained more in my main comment).

No action needed right now, just thinking out-loud. I think the proper thing to do here is apply these "common driver transformations" here, and store/use the flags as is going forward inside clangd, without extra mangling.

[DmitryPolukhin]

In D143436#4122594, @kadircet wrote:

That's actually because we model compile commands pushed via LSP as a "fixed compilation database" rather than a json compilation database (you can check the code in parseFixed, near parseJson).
The reason behind the discrepancy between fixed and json compilation databases mostly arises from the fact that the former is written by people specifically to be used by clang-tooling, whereas the latter is usually provided by build system and doesn't necessarily have the same concerns as clang-tooling hence requires certain modifications.

Thank you for sharing the context that I didn't know. I think it makes some sense but it requires duplication of this logic in some other places and keeping them up-to-date with driver changes.

That being said, the two particular transformations (response files & target/mode inference) seem like outliers when it comes to such transformations. These are handled by clang-driver binary, outside of the driver library, in https://github.com/llvm/llvm-project/blob/main/clang/tools/driver/driver.cpp#L426 and https://github.com/llvm/llvm-project/blob/main/clang/tools/driver/driver.cpp#L534. Therefore all the other tools that wants to make use of clang infra tries to simulate this logic, at different layers with slight differences (at the bare minimum, as the logic isn't shared and people mostly care about the driver, logic in other places just gets out-of-date).

I think clangd should be able to make this transformations if they are required even for "fixed" flags and it shouldn't change anything if compilation flags don't require them.

I believe the right thing to do here is putting all that arg preparation logic into driver library itself, so that all code paths that tries to create a compiler invocation can have the same behaviour. Unfortunately this is likely too hard to pull at this stage, due to people relying on almost every implementation detail of these interfaces.
So a middle ground would probably involve, moving that logic inside driver binary to a common library, so that future users can at least directly use it rather than trying to come up with their own interpretation (or trying to chose from existing N patterns) and we'll get rid of the issues that result from logic in this place and its duplicates getting out-of-sync. This way we don't need to migrate all the applications that want to create a compiler invocation to a new API and can only migrate clangd (CommandMangler is I believe the right layer for these purposes. as it handles all "string"-like modifications on the compile flags today).

Does that make sense? Is it something you'd like to propose a patch for? Because as things stand I think we're just making the matter worse here by introducing some new code paths that're trying to emulate the logic in driver today and will get out of sync at some point.

I see your point and it does make sense to me. I'll try to move logic to CommandMangler instead so both codepaths will share it. As for keeping up with driver changes, it is real issue and I'm not sure that we can solve this problem without using driver code almost "as is" that is not possible without serious refactoring. I'll ping you when changes are ready for review.

clang-tools-extra/clangd/ClangdLSPServer.cpp

1350 ↗

(On Diff #495431)

I think it won't hurt anything and it was not very useful even before my change. Now it just prevent small extra work when you set the same arguments more than one on arguments are very simple so no transformation actually happening.

[DmitryPolukhin]

Move standard adaptors to CommandMangler

[nridge]

clang-tools-extra/clangd/CompileCommands.cpp

216

The target needs to be added after the call to SystemIncludeExtractor later in this function (this is what D138546 is trying to fix). The reason is that SystemIncludeExtractor includes any --target flag in the compiler driver being queried for system includes, which may be gcc, which does not support --target.

[nridge]

clang-tools-extra/clangd/CompileCommands.cpp

216

(I guess we could make that change separately in D138546, but if we're changing the place where the --target is added in this patch, I figure we might as well move it directly to the desired place.)

[DmitryPolukhin]

Add test for expanded response files

[DmitryPolukhin]

@nridge I'm sorry, I pushed version without all tests. Now I added test case for response files, it seems that clangd hasn't had it before this patch.

clang-tools-extra/clangd/CompileCommands.cpp

216

I think there are order problems here:

• we need --driver-mode=cl injected here to make check on line #229 work as expected
• if we don't inject it driver mode here, command line edits won't see the change; see how I modified test clangd/unittests/CompileCommandsTests.cpp line #203, with D138546 edits were not applied properly to driver mode

I'll double check how it works on Windows but I expect that moving it after SystemIncludeExtractor will break proper driver mode detection.

[DmitryPolukhin]

Update test

[DmitryPolukhin]

Rebase and retest, cannot reproduce test failure locally

[DmitryPolukhin]

@nridge @kadircet please take another look, all tests now pass. As for the location of inserting driver mode please see my previous comment about MSVC CL check.

[nridge]

clang-tools-extra/clangd/CompileCommands.cpp

216

I think there are order problems here:

• we need --driver-mode=cl injected here to make check on line #229 work as expected

Looking at the line in question, it calls driver::getDriverMode() which falls back on extracting the driver mode from the program name anyways -- so I think that should be fine.

• if we don't inject it driver mode here, command line edits won't see the change; see how I modified test clangd/unittests/CompileCommandsTests.cpp line #203, with D138546 edits were not applied properly to driver mode

I'm not following the motivation for this test. Is there any real-world use case which requires the command line edits seeing the --driver-mode parameter?

[DmitryPolukhin]

clang-tools-extra/clangd/CompileCommands.cpp

216

I'm not following the motivation for this test. Is there any real-world use case which requires the command line edits seeing the --driver-mode parameter?

@nridge, it will break existing behaviour when user was able to remove these inserted options like this CompileFlags: {Remove: ['--driver-mode=*', '--target=*']}. If I move call of addTargetAndModeForProgramName after SystemIncludeExtractorr, inserted options will stay in list. User may only override it with CompileFlags.Compiler but it will override all compilers. I don't know is it real problem or not so if you think I should still move it after SystemIncludeExtractor, I'll do it.

[nridge]

clang-tools-extra/clangd/CompileCommands.cpp

216

@nridge, it will break existing behaviour when user was able to remove these inserted options like this CompileFlags: {Remove: ['--driver-mode=*', '--target=*']}.

Thanks. I'll leave the final word to @kadircet here but in my opinion, being able to remove those flags via Remove is not important; as you say, the user can change the Compiler to influence the driver mode if needed.

[DmitryPolukhin]

Move call addTargetAndModeForProgramName after SystemIncludeExtractor

[DmitryPolukhin]

Failing tests seems to be just flacky, I was not able to reprocuce any issue with them locally in stress runs and under ASan and TSan

[DmitryPolukhin]

Rebase

[DmitryPolukhin]

rebase

[DmitryPolukhin]

@kadircet friendly ping, could you please take a look to this diff?

[DmitryPolukhin]

Rebase

[DmitryPolukhin]

@kadircet and @nridge friendly ping could you please accept this diff or let me know if you would like to change anything here?
I moved addTargetAndModeForProgramName after SystemIncludeExtractor as was suggested.

[DmitryPolukhin]

@kadircet it is obvious that there is something in this diff that causes this hesitancy in accepting it. I'm ready to keep iterating on the solution but I need a clue what needs the improvement. Please comment.

[kadircet]

@kadircet it is obvious that there is something in this diff that causes this hesitancy in accepting it. I'm ready to keep iterating on the solution but I need a clue what needs the improvement. Please comment.

sorry for the late reply, i was on vacation and just started catching up with things. this looks great, thanks!

clang-tools-extra/clangd/CompileCommands.cpp

199

getting real filesystem is cheap, no need to make this part of the state, just inline it into tooling::addExpandedResponseFiles call.

216

agreed. addTargetAndModeForProgramName will add these flags only when they're not present. deleting --driver-mode or --target, without providing a fixed replacement doesn't sound like a real use case.

clang-tools-extra/clangd/test/Inputs/did-change-configuration-params.args

1 ↗

(On Diff #502110)

you can test expandResponseFile behaviour through unittests as well, see https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp#L133 for an example. i don't think complications in this lit test are worth it. do you mind moving this to a unittest?

clang-tools-extra/clangd/unittests/ClangdTests.cpp

1240 ↗

(On Diff #502110)

this feels unrelated, can you revert?

clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp

53

can you include a target triplet in the compiler name to test out target inference also works as intended?

clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp

53

you can drop tooling::

[DmitryPolukhin]

Comments resolved

[DmitryPolukhin]

@kadircet thank you for the review! Please take another look.

clang-tools-extra/clangd/CompileCommands.cpp

199

We need version of tooling::addExpandedResponseFiles with FS as an argument for testing at least, see https://github.com/llvm/llvm-project/blob/main/clang/unittests/Tooling/CompilationDatabaseTest.cpp#L968 switching it to using real filesystem IMHO makes things worse. So moved this code to the call site.

[DmitryPolukhin]

Fix test on Windows

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/CompileCommands.cpp

199

So moved this code to the call site.

I was also trying to say that. so all good.

[DmitryPolukhin]

Had to revert this diff temporary due to broken ARM builds. It seems that X86 targets are disabled for that bots so tests are not passing. I'll fix upload new version for review.

[DmitryPolukhin]

Update test to use any available target

[DmitryPolukhin]

@kadircet could you please take another look?
I had to update diff to make it work when LLVM is configured without X86 targets so they are not valid and thus not injected into the command line.

[asb]

This broke -DBUILD_SHARED_LIBS=True builds - I committed rG482d22d05a4a30a4f8594273bd359f7d311c9d4c to fix it.

[dyung]

@DmitryPolukhin your change seems to be causing a crash in 2 ORC tests on a bot (trivial-cxa-atexit.S and trivial-atexit.S), can you take a look or revert if you need time to investigate?

https://lab.llvm.org/buildbot/#/builders/247/builds/2639

******************** TEST 'ORC-x86_64-linux :: TestCases/Linux/x86-64/trivial-cxa-atexit.S' FAILED ********************
Script:
--
: 'RUN: at line 3';      /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./bin/clang   -m64  -c -o /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-cxa-atexit.S.tmp /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-cxa-atexit.S
: 'RUN: at line 4';   /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./bin/llvm-jitlink -orc-runtime=/home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./lib/clang/17/lib/x86_64-unknown-linux-gnu/liborc_rt.a /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-cxa-atexit.S.tmp
--
Exit Code: 139
Command Output (stderr):
--
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./bin/llvm-jitlink -orc-runtime=/home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./lib/clang/17/lib/x86_64-unknown-linux-gnu/liborc_rt.a /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-cxa-atexit.S.tmp
#0 0x00005654e9a19e86 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./bin/llvm-jitlink+0x10aae86)
#1 0x00005654e9a17534 SignalHandler(int) Signals.cpp:0:0
#2 0x00007f57b6259420 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14420)
#3 0x00007f57758df13a 
#4 0x00007f57758e27bc 
#5 0x00007f57758f102a 
#6 0x00007f57758e875f 
#7 0x00007f57758f4de2 
/home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-cxa-atexit.S.script: line 2: 3620821 Segmentation fault      (core dumped) /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./bin/llvm-jitlink -orc-runtime=/home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./lib/clang/17/lib/x86_64-unknown-linux-gnu/liborc_rt.a /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-cxa-atexit.S.tmp

[DmitryPolukhin]

In D143436#4203008, @dyung wrote:

@DmitryPolukhin your change seems to be causing a crash in 2 ORC tests on a bot (trivial-cxa-atexit.S and trivial-atexit.S), can you take a look or revert if you need time to investigate?

My changes haven't touched anything in compiler-rt at all but I'm looking into the issue.

[DmitryPolukhin]

@dyung I cannot reproduce this issue on two different machines in both cases tests work fine. For clang and llvm-jitlink binaries (the only binaries used in the tests) my changes are non-functional and I didn't change anything in compiler-rt or ORC. Are you able to reproduce the crashes locally?

[dyung]

In D143436#4203394, @DmitryPolukhin wrote:

@dyung I cannot reproduce this issue on two different machines in both cases tests work fine. For clang and llvm-jitlink binaries (the only binaries used in the tests) my changes are non-functional and I didn't change anything in compiler-rt or ORC. Are you able to reproduce the crashes locally?

So far I cannot on my own dev machines. I'll take the buildbot offline and try to see if I can figure out what is going on this weekend.

[dyung]

In D143436#4203525, @dyung wrote:

In D143436#4203394, @DmitryPolukhin wrote:

@dyung I cannot reproduce this issue on two different machines in both cases tests work fine. For clang and llvm-jitlink binaries (the only binaries used in the tests) my changes are non-functional and I didn't change anything in compiler-rt or ORC. Are you able to reproduce the crashes locally?

So far I cannot on my own dev machines. I'll take the buildbot offline and try to see if I can figure out what is going on this weekend.

So after deleting the build directory and clearing the ccache, the problem no longer seems to occur. I'm guessing it might have been corruption somewhere. Sorry about that!

[mgorny]

This commit broke building against libclang-cpp.so dylib. I've submitted a fix as D146427 but it haven't received any attention so far.

clang-tools-extra/clangd/CompileCommands.h

 //===--- CompileCommands.h - Manipulation of compile flags -------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILECOMMANDS_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILECOMMANDS_H
 
 #include "GlobalCompilationDatabase.h"
 #include "support/Threading.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/CommandLine.h"
 #include <deque>
 #include <optional>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
struct CommandMangler {
   static CommandMangler detect();
 
   // `Cmd` may describe compilation of a different file, and will be updated
   // for parsing `TargetFile`.
   void operator()(tooling::CompileCommand &Cmd,
                   llvm::StringRef TargetFile) const;
 
 private:
-  CommandMangler() = default;
+  CommandMangler();
 
   Memoize<llvm::StringMap<std::string>> ResolvedDrivers;
   Memoize<llvm::StringMap<std::string>> ResolvedDriversNoFollow;
+  llvm::cl::TokenizerCallback Tokenizer;
 };
 
 // Removes args from a command-line in a semantically-aware way.
 //
 // Internally this builds a large (0.5MB) table of clang options on first use.
 // Both strip() and process() are fairly cheap after that.
 //
 // FIXME: this reimplements much of OptTable, it might be nice to expose more.

clang-tools-extra/clangd/CompileCommands.cpp

 //===--- CompileCommands.cpp ----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "CompileCommands.h"
 #include "Config.h"
 #include "support/Logger.h"
 #include "support/Trace.h"
 #include "clang/Driver/Driver.h"
 #include "clang/Driver/Options.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Tooling/CompilationDatabase.h"
+#include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
+#include "llvm/TargetParser/Host.h"
 #include <iterator>
 #include <optional>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
if (FollowSymlink) {
     if (!llvm::sys::fs::real_path(Driver, Resolved))
       return SiblingOf(Resolved);
   }
   return Driver.str();
 }
 
 } // namespace
 
+CommandMangler::CommandMangler() {
+  Tokenizer = llvm::Triple(llvm::sys::getProcessTriple()).isOSWindows()
+                  ? llvm::cl::TokenizeWindowsCommandLine
+                  : llvm::cl::TokenizeGNUCommandLine;
+}
+
 CommandMangler CommandMangler::detect() {
   CommandMangler Result;
   Result.ClangPath = detectClangPath();
   Result.ResourceDir = detectStandardResourceDir();
   Result.Sysroot = detectSysroot();
   return Result;
 }
 
 CommandMangler CommandMangler::forTests() { return CommandMangler(); }
 
 void CommandMangler::operator()(tooling::CompileCommand &Command,
                                 llvm::StringRef File) const {
   std::vector<std::string> &Cmd = Command.CommandLine;
   trace::Span S("AdjustCompileFlags");
   // Most of the modifications below assumes the Cmd starts with a driver name.
   // We might consider injecting a generic driver name like "cc" or "c++", but
-  // a Cmd missing the driver is probably rare enough in practice and errnous.
+  // a Cmd missing the driver is probably rare enough in practice and erroneous.
   if (Cmd.empty())
     return;
+
+  // FS used for expanding response files.
+  // FIXME: ExpandResponseFiles appears not to provide the usual
+  // thread-safety guarantees, as the access to FS is not locked!
+  // For now, use the real FS, which is known to be threadsafe (if we don't
+  // use/change working directory, which ExpandResponseFiles doesn't).
+  auto FS = llvm::vfs::getRealFileSystem();
+  tooling::addExpandedResponseFiles(Cmd, Command.Directory, Tokenizer, *FS);
+
   auto &OptTable = clang::driver::getDriverOptTable();
   // OriginalArgs needs to outlive ArgList.
   llvm::SmallVector<const char *, 16> OriginalArgs;
   OriginalArgs.reserve(Cmd.size());
   for (const auto &S : Cmd)
     OriginalArgs.push_back(S.c_str());
   bool IsCLMode = driver::IsClangCL(driver::getDriverMode(
       OriginalArgs[0], llvm::ArrayRef(OriginalArgs).slice(1)));
-  // ParseArgs propagates missig arg/opt counts on error, but preserves
+  // ParseArgs propagates missing arg/opt counts on error, but preserves
   // everything it could parse in ArgList. So we just ignore those counts.
   unsigned IgnoredCount;
   // Drop the executable name, as ParseArgs doesn't expect it. This means
   // indices are actually of by one between ArgList and OriginalArgs.
   llvm::opt::InputArgList ArgList;
   ArgList = OptTable.ParseArgs(
       llvm::ArrayRef(OriginalArgs).drop_front(), IgnoredCount, IgnoredCount,
       /*FlagsToInclude=*/
void CommandMangler::operator()(tooling::CompileCommand &Command,
   for (auto &Edit : Config::current().CompileFlags.Edits)
     Edit(Cmd);
 
   // The system include extractor needs to run:
   //  - AFTER transferCompileCommand(), because the -x flag it adds may be
   //    necessary for the system include extractor to identify the file type
   //  - AFTER applying CompileFlags.Edits, because the name of the compiler
   //    that needs to be invoked may come from the CompileFlags->Compiler key
+  //  - BEFORE addTargetAndModeForProgramName(), because gcc doesn't support
+  //    the target flag that might be added.
   //  - BEFORE resolveDriver() because that can mess up the driver path,
   //    e.g. changing gcc to /path/to/clang/bin/gcc
   if (SystemIncludeExtractor) {
     SystemIncludeExtractor(Command, File);
   }
 
+  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;
   };

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

DirectoryBasedGlobalCompilationDatabase::DirectoryCache::CachedFile::load(
   return {LoadResult::FoundNewData, std::move(*Buf)};
 }
 
 // Adapt CDB-loading functions to a common interface for DirectoryCache::load().
 static std::unique_ptr<tooling::CompilationDatabase>
 parseJSON(PathRef Path, llvm::StringRef Data, std::string &Error) {
   if (auto CDB = tooling::JSONCompilationDatabase::loadFromBuffer(
           Data, Error, tooling::JSONCommandLineSyntax::AutoDetect)) {
-    // FS used for expanding response files.
-    // FIXME: ExpandResponseFilesDatabase appears not to provide the usual
-    // thread-safety guarantees, as the access to FS is not locked!
-    // For now, use the real FS, which is known to be threadsafe (if we don't
-    // use/change working directory, which ExpandResponseFilesDatabase doesn't).
-    auto FS = llvm::vfs::getRealFileSystem();
-    return tooling::inferTargetAndDriverMode(
-        tooling::inferMissingCompileCommands(
-            expandResponseFiles(std::move(CDB), std::move(FS))));
+    return tooling::inferMissingCompileCommands(std::move(CDB));
   }
   return nullptr;
 }
 static std::unique_ptr<tooling::CompilationDatabase>
 parseFixed(PathRef Path, llvm::StringRef Data, std::string &Error) {
   return tooling::FixedCompilationDatabase::loadFromBuffer(
       llvm::sys::path::parent_path(Path), Data, Error);
 }

clang-tools-extra/clangd/unittests/CMakeLists.txt

clang_target_link_libraries(ClangdTests
   clangASTMatchers
   clangBasic
   clangFormat
   clangFrontend
   clangIndex
   clangLex
   clangSema
   clangSerialization
+  clangTesting
   clangTooling
   clangToolingCore
   clangToolingInclusions
   clangToolingInclusionsStdlib
   clangToolingRefactoring
   clangToolingSyntax
   )
 target_link_libraries(ClangdTests

clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp

 //===-- CompileCommandsTests.cpp ------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "CompileCommands.h"
 #include "Config.h"
 #include "TestFS.h"
 #include "support/Context.h"
 
+#include "clang/Testing/CommandLineArgs.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
+#include "llvm/Support/TargetSelect.h"
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 using ::testing::_;
 using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::HasSubstr;
 using ::testing::Not;
 
 // Sadly, CommandMangler::detect(), which contains much of the logic, is
 // a bunch of untested integration glue. We test the string manipulation here
 // assuming its results are correct.
 
 // Make use of all features and assert the exact command we get out.
 // Other tests just verify presence/absence of certain args.
 TEST(CommandMangler, Everything) {
+  llvm::InitializeAllTargetInfos(); // As in ClangdMain
+  std::string Target = getAnyTargetForTesting();
   auto Mangler = CommandMangler::forTests();
   Mangler.ClangPath = testPath("fake/clang");
   Mangler.ResourceDir = testPath("fake/resources");
   Mangler.Sysroot = testPath("fake/sysroot");
   tooling::CompileCommand Cmd;
-  Cmd.CommandLine = {"clang++", "--", "foo.cc", "bar.cc"};
+  Cmd.CommandLine = {Target + "-clang++", "--", "foo.cc", "bar.cc"};
   Mangler(Cmd, "foo.cc");
   EXPECT_THAT(Cmd.CommandLine,
-              ElementsAre(testPath("fake/clang++"),
+              ElementsAre(testPath("fake/" + Target + "-clang++"),
+                          "--target=" + Target, "--driver-mode=g++",
                           "-resource-dir=" + testPath("fake/resources"),
                           "-isysroot", testPath("fake/sysroot"), "--",
                           "foo.cc"));
 }
 
 TEST(CommandMangler, FilenameMismatch) {
   auto Mangler = CommandMangler::forTests();
   Mangler.ClangPath = testPath("clang");
Cmd.CommandLine = {"clang++", "foo.cc"};
     });
     Cfg.CompileFlags.Edits.push_back([](std::vector<std::string> &Argv) {
       Argv = tooling::getInsertArgumentAdjuster("--hello")(Argv, "");
     });
     WithContextValue WithConfig(Config::Key, std::move(Cfg));
     Mangler(Cmd, "foo.cc");
   }
   // Edits are applied in given order and before other mangling and they always
-  // go before filename.
-  EXPECT_THAT(Cmd.CommandLine, ElementsAre(_, "--hello", "--", "FOO.CC"));
+  // go before filename. `--driver-mode=g++` here is in lower case because
+  // options inserted by addTargetAndModeForProgramName are not editable,
+  // see discussion in https://reviews.llvm.org/D138546
+  EXPECT_THAT(Cmd.CommandLine,
+              ElementsAre(_, "--driver-mode=g++", "--hello", "--", "FOO.CC"));
+}
+
+TEST(CommandMangler, ExpandedResponseFiles) {
+  SmallString<1024> Path;
+  int FD;
+  ASSERT_FALSE(llvm::sys::fs::createTemporaryFile("args", "", FD, Path));
+  llvm::raw_fd_ostream OutStream(FD, true);
+  OutStream << "-Wall";
+  OutStream.close();
+
+  auto Mangler = CommandMangler::forTests();
+  tooling::CompileCommand Cmd;
+  Cmd.CommandLine = {"clang", ("@" + Path).str(), "foo.cc"};
+  Mangler(Cmd, "foo.cc");
+  EXPECT_THAT(Cmd.CommandLine, ElementsAre(_, "-Wall", "--", "foo.cc"));
 }
 
 static std::string strip(llvm::StringRef Arg, llvm::StringRef Argv) {
   llvm::SmallVector<llvm::StringRef> Parts;
   llvm::SplitString(Argv, Parts);
   std::vector<std::string> Args = {Parts.begin(), Parts.end()};
   ArgStripper S;
   S.strip(Arg);

clang/include/clang/Testing/CommandLineArgs.h

enum TestLanguage {
   Lang_OBJCXX
 };
 
 std::vector<std::string> getCommandLineArgsForTesting(TestLanguage Lang);
 std::vector<std::string> getCC1ArgsForTesting(TestLanguage Lang);
 
 StringRef getFilenameForTesting(TestLanguage Lang);
 
+/// Find a target name such that looking for it in TargetRegistry by that name
+/// returns the same target. We expect that there is at least one target
+/// configured with this property.
+std::string getAnyTargetForTesting();
+
 } // end namespace clang
 
 #endif

clang/include/clang/Tooling/Tooling.h

 /// \param InvokedAs the path to the driver used to infer implicit flags.
 ///
 /// \note This will not set \c CommandLine[0] to \c InvokedAs. The tooling
 /// infrastructure expects that CommandLine[0] is a tool path relative to which
 /// the builtin headers can be found.
 void addTargetAndModeForProgramName(std::vector<std::string> &CommandLine,
                                     StringRef InvokedAs);
 
+/// Helper function that expands response files in command line.
+void addExpandedResponseFiles(std::vector<std::string> &CommandLine,
+                              llvm::StringRef WorkingDir,
+                              llvm::cl::TokenizerCallback Tokenizer,
+                              llvm::vfs::FileSystem &FS);
+
 /// Creates a \c CompilerInvocation.
 CompilerInvocation *newInvocation(DiagnosticsEngine *Diagnostics,
                                   ArrayRef<const char *> CC1Args,
                                   const char *const BinaryName);
 
 } // namespace tooling
 
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLING_TOOLING_H

clang/lib/Testing/CommandLineArgs.cpp

 //===--- CommandLineArgs.cpp ----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Testing/CommandLineArgs.h"
+#include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/ErrorHandling.h"
 
 namespace clang {
 
 std::vector<std::string> getCommandLineArgsForTesting(TestLanguage Lang) {
   std::vector<std::string> Args;
   // Test with basic arguments.
   switch (Lang) {
case Lang_OBJC:
     return "input.m";
 
   case Lang_OBJCXX:
     return "input.mm";
   }
   llvm_unreachable("Unhandled TestLanguage enum");
 }
 
+std::string getAnyTargetForTesting() {
+  for (const auto &Target : llvm::TargetRegistry::targets()) {
+    std::string Error;
+    StringRef TargetName(Target.getName());
+    if (TargetName == "x86-64")
+      TargetName = "x86_64";
+    if (llvm::TargetRegistry::lookupTarget(std::string(TargetName), Error) ==
+        &Target) {
+      return std::string(TargetName);
+    }
+  }
+  return "";
+}
+
 } // end namespace clang

clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp

 //===- ExpandResponseFileCompilationDataBase.cpp --------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Tooling/CompilationDatabase.h"
+#include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/StringSaver.h"
 #include "llvm/TargetParser/Host.h"
public:
   }
 
   std::vector<CompileCommand> getAllCompileCommands() const override {
     return expand(Base->getAllCompileCommands());
   }
 
 private:
   std::vector<CompileCommand> expand(std::vector<CompileCommand> Cmds) const {
-    for (auto &Cmd : Cmds) {
-      bool SeenRSPFile = false;
-      llvm::SmallVector<const char *, 20> Argv;
-      Argv.reserve(Cmd.CommandLine.size());
-      for (auto &Arg : Cmd.CommandLine) {
-        Argv.push_back(Arg.c_str());
-        if (!Arg.empty())
-          SeenRSPFile |= Arg.front() == '@';
-      }
-      if (!SeenRSPFile)
-        continue;
-      llvm::BumpPtrAllocator Alloc;
-      llvm::cl::ExpansionContext ECtx(Alloc, Tokenizer);
-      llvm::Error Err = ECtx.setVFS(FS.get())
-                            .setCurrentDir(Cmd.Directory)
-                            .expandResponseFiles(Argv);
-      if (Err)
-        llvm::errs() << Err;
-      // Don't assign directly, Argv aliases CommandLine.
-      std::vector<std::string> ExpandedArgv(Argv.begin(), Argv.end());
-      Cmd.CommandLine = std::move(ExpandedArgv);
-    }
+    for (auto &Cmd : Cmds)
+      tooling::addExpandedResponseFiles(Cmd.CommandLine, Cmd.Directory,
+                                        Tokenizer, *FS);
     return Cmds;
   }
 
 private:
   std::unique_ptr<CompilationDatabase> Base;
   llvm::cl::TokenizerCallback Tokenizer;
   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS;
 };

clang/lib/Tooling/Tooling.cpp

 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/OptTable.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/Casting.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/TargetParser/Host.h"
if (ShouldAddMode) {
     CommandLine.insert(++CommandLine.begin(), TargetMode.DriverMode);
   }
   if (ShouldAddTarget) {
     CommandLine.insert(++CommandLine.begin(),
                        TargetOPT + TargetMode.TargetPrefix);
   }
 }
 
+void addExpandedResponseFiles(std::vector<std::string> &CommandLine,
+                              llvm::StringRef WorkingDir,
+                              llvm::cl::TokenizerCallback Tokenizer,
+                              llvm::vfs::FileSystem &FS) {
+  bool SeenRSPFile = false;
+  llvm::SmallVector<const char *, 20> Argv;
+  Argv.reserve(CommandLine.size());
+  for (auto &Arg : CommandLine) {
+    Argv.push_back(Arg.c_str());
+    if (!Arg.empty())
+      SeenRSPFile |= Arg.front() == '@';
+  }
+  if (!SeenRSPFile)
+    return;
+  llvm::BumpPtrAllocator Alloc;
+  llvm::cl::ExpansionContext ECtx(Alloc, Tokenizer);
+  llvm::Error Err =
+      ECtx.setVFS(&FS).setCurrentDir(WorkingDir).expandResponseFiles(Argv);
+  if (Err)
+    llvm::errs() << Err;
+  // Don't assign directly, Argv aliases CommandLine.
+  std::vector<std::string> ExpandedArgv(Argv.begin(), Argv.end());
+  CommandLine = std::move(ExpandedArgv);
+}
+
 } // namespace tooling
 } // namespace clang
 
 namespace {
 
 class SingleFrontendActionFactory : public FrontendActionFactory {
   std::unique_ptr<FrontendAction> Action;
 
std::unique_ptr<ASTUnit> buildASTFromCodeWithArgs(
   for (auto &FilenameWithContent : VirtualMappedFiles) {
     InMemoryFileSystem->addFile(
         FilenameWithContent.first, 0,
         llvm::MemoryBuffer::getMemBuffer(FilenameWithContent.second));
   }
 
   if (!Invocation.run())
     return nullptr;
- 
   assert(ASTs.size() == 1);
   return std::move(ASTs[0]);
 }
 
 } // namespace tooling
 } // namespace clang

clang/unittests/Tooling/ToolingTest.cpp

 #include "clang/AST/DeclGroup.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/Driver.h"
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendAction.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Frontend/TextDiagnosticBuffer.h"
+#include "clang/Testing/CommandLineArgs.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/TargetParser/Host.h"
 #include "gtest/gtest.h"
 #include <algorithm>
 #include <string>
 #include <vector>
 
TEST(ClangToolTest, StripPluginsAdjuster) {
   auto HasFlag = [&FinalArgs](const std::string &Flag) {
     return llvm::find(FinalArgs, Flag) != FinalArgs.end();
   };
   EXPECT_FALSE(HasFlag("-Xclang"));
   EXPECT_FALSE(HasFlag("-add-plugin"));
   EXPECT_FALSE(HasFlag("-random-plugin"));
 }
 
-namespace {
-/// Find a target name such that looking for it in TargetRegistry by that name
-/// returns the same target. We expect that there is at least one target
-/// configured with this property.
-std::string getAnyTarget() {
+TEST(addTargetAndModeForProgramName, AddsTargetAndMode) {
   llvm::InitializeAllTargets();
-  for (const auto &Target : llvm::TargetRegistry::targets()) {
-    std::string Error;
-    StringRef TargetName(Target.getName());
-    if (TargetName == "x86-64")
-      TargetName = "x86_64";
-    if (llvm::TargetRegistry::lookupTarget(std::string(TargetName), Error) ==
-        &Target) {
-      return std::string(TargetName);
-    }
-  }
-  return "";
-}
-}
 
-TEST(addTargetAndModeForProgramName, AddsTargetAndMode) {
-  std::string Target = getAnyTarget();
+  std::string Target = getAnyTargetForTesting();
   ASSERT_FALSE(Target.empty());
 
   std::vector<std::string> Args = {"clang", "-foo"};
   addTargetAndModeForProgramName(Args, "");
   EXPECT_EQ((std::vector<std::string>{"clang", "-foo"}), Args);
   addTargetAndModeForProgramName(Args, Target + "-g++");
   EXPECT_EQ((std::vector<std::string>{"clang", "--target=" + Target,
                                       "--driver-mode=g++", "-foo"}),
             Args);
 }
 
 TEST(addTargetAndModeForProgramName, PathIgnored) {
-  std::string Target = getAnyTarget();
+  llvm::InitializeAllTargets();
+  std::string Target = getAnyTargetForTesting();
   ASSERT_FALSE(Target.empty());
 
   SmallString<32> ToolPath;
   llvm::sys::path::append(ToolPath, "foo", "bar", Target + "-g++");
 
   std::vector<std::string> Args = {"clang", "-foo"};
   addTargetAndModeForProgramName(Args, ToolPath);
   EXPECT_EQ((std::vector<std::string>{"clang", "--target=" + Target,
                                       "--driver-mode=g++", "-foo"}),
             Args);
 }
 
 TEST(addTargetAndModeForProgramName, IgnoresExistingTarget) {
-  std::string Target = getAnyTarget();
+  llvm::InitializeAllTargets();
+  std::string Target = getAnyTargetForTesting();
   ASSERT_FALSE(Target.empty());
 
   std::vector<std::string> Args = {"clang", "-foo", "-target", "something"};
   addTargetAndModeForProgramName(Args, Target + "-g++");
   EXPECT_EQ((std::vector<std::string>{"clang", "--driver-mode=g++", "-foo",
                                       "-target", "something"}),
             Args);
 
   std::vector<std::string> ArgsAlt = {"clang", "-foo", "--target=something"};
   addTargetAndModeForProgramName(ArgsAlt, Target + "-g++");
   EXPECT_EQ((std::vector<std::string>{"clang", "--driver-mode=g++", "-foo",
                                       "--target=something"}),
             ArgsAlt);
 }
 
 TEST(addTargetAndModeForProgramName, IgnoresExistingMode) {
-  std::string Target = getAnyTarget();
+  llvm::InitializeAllTargets();
+  std::string Target = getAnyTargetForTesting();
   ASSERT_FALSE(Target.empty());
 
   std::vector<std::string> Args = {"clang", "-foo", "--driver-mode=abc"};
   addTargetAndModeForProgramName(Args, Target + "-g++");
   EXPECT_EQ((std::vector<std::string>{"clang", "--target=" + Target, "-foo",
                                       "--driver-mode=abc"}),
             Args);
 }