[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin updated this revision to Diff 499433.
DmitryPolukhin added a comment.

Move call addTargetAndModeForProgramName after SystemIncludeExtractor


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436

Files:
  clang-tools-extra/clangd/CompileCommands.cpp
  clang-tools-extra/clangd/CompileCommands.h
  clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
  clang-tools-extra/clangd/test/Inputs/did-change-configuration-params.args
  clang-tools-extra/clangd/test/did-change-configuration-params.test
  clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
  clang/include/clang/Tooling/Tooling.h
  clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
  clang/lib/Tooling/Tooling.cpp

Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -43,6 +43,7 @@
 #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"
@@ -299,6 +300,31 @@
   }
 }
 
+void addExpandedResponseFiles(std::vector ,
+  llvm::StringRef WorkingDir,
+  llvm::cl::TokenizerCallback Tokenizer,
+  llvm::vfs::FileSystem ) {
+  bool SeenRSPFile = false;
+  llvm::SmallVector Argv;
+  Argv.reserve(CommandLine.size());
+  for (auto  : 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().setCurrentDir(WorkingDir).expandResponseFiles(Argv);
+  if (Err)
+llvm::errs() << Err;
+  // Don't assign directly, Argv aliases CommandLine.
+  std::vector ExpandedArgv(Argv.begin(), Argv.end());
+  CommandLine = std::move(ExpandedArgv);
+}
+
 } // namespace tooling
 } // namespace clang
 
@@ -684,7 +710,7 @@
 
   if (!Invocation.run())
 return nullptr;
- 
+
   assert(ASTs.size() == 1);
   return std::move(ASTs[0]);
 }
Index: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
===
--- clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
+++ clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #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"
@@ -48,28 +49,9 @@
 
 private:
   std::vector expand(std::vector Cmds) const {
-for (auto  : Cmds) {
-  bool SeenRSPFile = false;
-  llvm::SmallVector Argv;
-  Argv.reserve(Cmd.CommandLine.size());
-  for (auto  : 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 ExpandedArgv(Argv.begin(), Argv.end());
-  Cmd.CommandLine = std::move(ExpandedArgv);
-}
+for (auto  : Cmds)
+  tooling::addExpandedResponseFiles(Cmd.CommandLine, Cmd.Directory,
+Tokenizer, *FS);
 return Cmds;
   }
 
Index: clang/include/clang/Tooling/Tooling.h
===
--- clang/include/clang/Tooling/Tooling.h
+++ clang/include/clang/Tooling/Tooling.h
@@ -506,6 +506,12 @@
 void addTargetAndModeForProgramName(std::vector ,
 StringRef InvokedAs);
 
+/// Helper function that expands response files in command line.
+void addExpandedResponseFiles(std::vector ,
+  llvm::StringRef WorkingDir,
+  llvm::cl::TokenizerCallback Tokenizer,
+  llvm::vfs::FileSystem );
+
 /// Creates a \c CompilerInvocation.
 CompilerInvocation *newInvocation(DiagnosticsEngine *Diagnostics,
   ArrayRef CC1Args,
Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ 

[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/CompileCommands.cpp:222
+  tooling::addExpandedResponseFiles(Cmd, Command.Directory, Tokenizer, *FS);
+  tooling::addTargetAndModeForProgramName(Cmd, Cmd.front());
   auto  = clang::driver::getDriverOptTable();

DmitryPolukhin wrote:
> nridge wrote:
> > DmitryPolukhin wrote:
> > > nridge wrote:
> > > > nridge wrote:
> > > > > 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`.
> > > > (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.)
> > > 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.
> > > 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](https://searchfox.org/llvm/rev/0cbb8ec030e23c0e13331b5d54155def8c901b36/clang-tools-extra/clangd/CompileCommands.cpp#213),
> >  it calls `driver::getDriverMode()` which [falls back on extracting the 
> > driver 
> > mode](https://searchfox.org/llvm/rev/0cbb8ec030e23c0e13331b5d54155def8c901b36/clang/lib/Driver/Driver.cpp#6407)
> >  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?
> > 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 
> `SystemIncludeExtractor`r, 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, 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436

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

[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin added inline comments.



Comment at: clang-tools-extra/clangd/CompileCommands.cpp:222
+  tooling::addExpandedResponseFiles(Cmd, Command.Directory, Tokenizer, *FS);
+  tooling::addTargetAndModeForProgramName(Cmd, Cmd.front());
   auto  = clang::driver::getDriverOptTable();

nridge wrote:
> DmitryPolukhin wrote:
> > nridge wrote:
> > > nridge wrote:
> > > > 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`.
> > > (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.)
> > 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.
> > 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](https://searchfox.org/llvm/rev/0cbb8ec030e23c0e13331b5d54155def8c901b36/clang-tools-extra/clangd/CompileCommands.cpp#213),
>  it calls `driver::getDriverMode()` which [falls back on extracting the 
> driver 
> mode](https://searchfox.org/llvm/rev/0cbb8ec030e23c0e13331b5d54155def8c901b36/clang/lib/Driver/Driver.cpp#6407)
>  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?
> 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 
`SystemIncludeExtractor`r, 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436

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

[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/CompileCommands.cpp:222
+  tooling::addExpandedResponseFiles(Cmd, Command.Directory, Tokenizer, *FS);
+  tooling::addTargetAndModeForProgramName(Cmd, Cmd.front());
   auto  = clang::driver::getDriverOptTable();

DmitryPolukhin wrote:
> nridge wrote:
> > nridge wrote:
> > > 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`.
> > (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.)
> 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.
> 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](https://searchfox.org/llvm/rev/0cbb8ec030e23c0e13331b5d54155def8c901b36/clang-tools-extra/clangd/CompileCommands.cpp#213),
 it calls `driver::getDriverMode()` which [falls back on extracting the driver 
mode](https://searchfox.org/llvm/rev/0cbb8ec030e23c0e13331b5d54155def8c901b36/clang/lib/Driver/Driver.cpp#6407)
 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436

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

[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin added a comment.

@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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436

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

[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin updated this revision to Diff 498878.
DmitryPolukhin added a comment.

Rebase and retest, cannot reproduce test failure locally


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436

Files:
  clang-tools-extra/clangd/CompileCommands.cpp
  clang-tools-extra/clangd/CompileCommands.h
  clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
  clang-tools-extra/clangd/test/Inputs/did-change-configuration-params.args
  clang-tools-extra/clangd/test/did-change-configuration-params.test
  clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
  clang/include/clang/Tooling/Tooling.h
  clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
  clang/lib/Tooling/Tooling.cpp

Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -43,6 +43,7 @@
 #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"
@@ -299,6 +300,31 @@
   }
 }
 
+void addExpandedResponseFiles(std::vector ,
+  llvm::StringRef WorkingDir,
+  llvm::cl::TokenizerCallback Tokenizer,
+  llvm::vfs::FileSystem ) {
+  bool SeenRSPFile = false;
+  llvm::SmallVector Argv;
+  Argv.reserve(CommandLine.size());
+  for (auto  : 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().setCurrentDir(WorkingDir).expandResponseFiles(Argv);
+  if (Err)
+llvm::errs() << Err;
+  // Don't assign directly, Argv aliases CommandLine.
+  std::vector ExpandedArgv(Argv.begin(), Argv.end());
+  CommandLine = std::move(ExpandedArgv);
+}
+
 } // namespace tooling
 } // namespace clang
 
@@ -684,7 +710,7 @@
 
   if (!Invocation.run())
 return nullptr;
- 
+
   assert(ASTs.size() == 1);
   return std::move(ASTs[0]);
 }
Index: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
===
--- clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
+++ clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #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"
@@ -48,28 +49,9 @@
 
 private:
   std::vector expand(std::vector Cmds) const {
-for (auto  : Cmds) {
-  bool SeenRSPFile = false;
-  llvm::SmallVector Argv;
-  Argv.reserve(Cmd.CommandLine.size());
-  for (auto  : 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 ExpandedArgv(Argv.begin(), Argv.end());
-  Cmd.CommandLine = std::move(ExpandedArgv);
-}
+for (auto  : Cmds)
+  tooling::addExpandedResponseFiles(Cmd.CommandLine, Cmd.Directory,
+Tokenizer, *FS);
 return Cmds;
   }
 
Index: clang/include/clang/Tooling/Tooling.h
===
--- clang/include/clang/Tooling/Tooling.h
+++ clang/include/clang/Tooling/Tooling.h
@@ -506,6 +506,12 @@
 void addTargetAndModeForProgramName(std::vector ,
 StringRef InvokedAs);
 
+/// Helper function that expands response files in command line.
+void addExpandedResponseFiles(std::vector ,
+  llvm::StringRef WorkingDir,
+  llvm::cl::TokenizerCallback Tokenizer,
+  llvm::vfs::FileSystem );
+
 /// Creates a \c CompilerInvocation.
 CompilerInvocation *newInvocation(DiagnosticsEngine *Diagnostics,
   ArrayRef CC1Args,
Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp

[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin updated this revision to Diff 498140.
DmitryPolukhin added a comment.

Update test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436

Files:
  clang-tools-extra/clangd/CompileCommands.cpp
  clang-tools-extra/clangd/CompileCommands.h
  clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
  clang-tools-extra/clangd/test/Inputs/did-change-configuration-params.args
  clang-tools-extra/clangd/test/did-change-configuration-params.test
  clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
  clang/include/clang/Tooling/Tooling.h
  clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
  clang/lib/Tooling/Tooling.cpp

Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -43,6 +43,7 @@
 #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"
@@ -299,6 +300,31 @@
   }
 }
 
+void addExpandedResponseFiles(std::vector ,
+  llvm::StringRef WorkingDir,
+  llvm::cl::TokenizerCallback Tokenizer,
+  llvm::vfs::FileSystem ) {
+  bool SeenRSPFile = false;
+  llvm::SmallVector Argv;
+  Argv.reserve(CommandLine.size());
+  for (auto  : 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().setCurrentDir(WorkingDir).expandResponseFiles(Argv);
+  if (Err)
+llvm::errs() << Err;
+  // Don't assign directly, Argv aliases CommandLine.
+  std::vector ExpandedArgv(Argv.begin(), Argv.end());
+  CommandLine = std::move(ExpandedArgv);
+}
+
 } // namespace tooling
 } // namespace clang
 
@@ -684,7 +710,7 @@
 
   if (!Invocation.run())
 return nullptr;
- 
+
   assert(ASTs.size() == 1);
   return std::move(ASTs[0]);
 }
Index: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
===
--- clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
+++ clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #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"
@@ -48,28 +49,9 @@
 
 private:
   std::vector expand(std::vector Cmds) const {
-for (auto  : Cmds) {
-  bool SeenRSPFile = false;
-  llvm::SmallVector Argv;
-  Argv.reserve(Cmd.CommandLine.size());
-  for (auto  : 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 ExpandedArgv(Argv.begin(), Argv.end());
-  Cmd.CommandLine = std::move(ExpandedArgv);
-}
+for (auto  : Cmds)
+  tooling::addExpandedResponseFiles(Cmd.CommandLine, Cmd.Directory,
+Tokenizer, *FS);
 return Cmds;
   }
 
Index: clang/include/clang/Tooling/Tooling.h
===
--- clang/include/clang/Tooling/Tooling.h
+++ clang/include/clang/Tooling/Tooling.h
@@ -506,6 +506,12 @@
 void addTargetAndModeForProgramName(std::vector ,
 StringRef InvokedAs);
 
+/// Helper function that expands response files in command line.
+void addExpandedResponseFiles(std::vector ,
+  llvm::StringRef WorkingDir,
+  llvm::cl::TokenizerCallback Tokenizer,
+  llvm::vfs::FileSystem );
+
 /// Creates a \c CompilerInvocation.
 CompilerInvocation *newInvocation(DiagnosticsEngine *Diagnostics,
   ArrayRef CC1Args,
Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -49,7 +49,7 @@
   Cmd.CommandLine = 

[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin added a comment.

@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.




Comment at: clang-tools-extra/clangd/CompileCommands.cpp:222
+  tooling::addExpandedResponseFiles(Cmd, Command.Directory, Tokenizer, *FS);
+  tooling::addTargetAndModeForProgramName(Cmd, Cmd.front());
   auto  = clang::driver::getDriverOptTable();

nridge wrote:
> nridge wrote:
> > 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`.
> (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.)
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436

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

[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin updated this revision to Diff 498138.
DmitryPolukhin added a comment.

Add test for expanded response files


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436

Files:
  clang-tools-extra/clangd/CompileCommands.cpp
  clang-tools-extra/clangd/CompileCommands.h
  clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
  clang-tools-extra/clangd/test/Inputs/did-change-configuration-params.args
  clang-tools-extra/clangd/test/did-change-configuration-params.test
  clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
  clang/include/clang/Tooling/Tooling.h
  clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
  clang/lib/Tooling/Tooling.cpp

Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -43,6 +43,7 @@
 #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"
@@ -299,6 +300,31 @@
   }
 }
 
+void addExpandedResponseFiles(std::vector ,
+  llvm::StringRef WorkingDir,
+  llvm::cl::TokenizerCallback Tokenizer,
+  llvm::vfs::FileSystem ) {
+  bool SeenRSPFile = false;
+  llvm::SmallVector Argv;
+  Argv.reserve(CommandLine.size());
+  for (auto  : 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().setCurrentDir(WorkingDir).expandResponseFiles(Argv);
+  if (Err)
+llvm::errs() << Err;
+  // Don't assign directly, Argv aliases CommandLine.
+  std::vector ExpandedArgv(Argv.begin(), Argv.end());
+  CommandLine = std::move(ExpandedArgv);
+}
+
 } // namespace tooling
 } // namespace clang
 
@@ -684,7 +710,7 @@
 
   if (!Invocation.run())
 return nullptr;
- 
+
   assert(ASTs.size() == 1);
   return std::move(ASTs[0]);
 }
Index: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
===
--- clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
+++ clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #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"
@@ -48,28 +49,9 @@
 
 private:
   std::vector expand(std::vector Cmds) const {
-for (auto  : Cmds) {
-  bool SeenRSPFile = false;
-  llvm::SmallVector Argv;
-  Argv.reserve(Cmd.CommandLine.size());
-  for (auto  : 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 ExpandedArgv(Argv.begin(), Argv.end());
-  Cmd.CommandLine = std::move(ExpandedArgv);
-}
+for (auto  : Cmds)
+  tooling::addExpandedResponseFiles(Cmd.CommandLine, Cmd.Directory,
+Tokenizer, *FS);
 return Cmds;
   }
 
Index: clang/include/clang/Tooling/Tooling.h
===
--- clang/include/clang/Tooling/Tooling.h
+++ clang/include/clang/Tooling/Tooling.h
@@ -506,6 +506,12 @@
 void addTargetAndModeForProgramName(std::vector ,
 StringRef InvokedAs);
 
+/// Helper function that expands response files in command line.
+void addExpandedResponseFiles(std::vector ,
+  llvm::StringRef WorkingDir,
+  llvm::cl::TokenizerCallback Tokenizer,
+  llvm::vfs::FileSystem );
+
 /// Creates a \c CompilerInvocation.
 CompilerInvocation *newInvocation(DiagnosticsEngine *Diagnostics,
   ArrayRef CC1Args,
Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -49,7 +49,7 @@
  

[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/CompileCommands.cpp:222
+  tooling::addExpandedResponseFiles(Cmd, Command.Directory, Tokenizer, *FS);
+  tooling::addTargetAndModeForProgramName(Cmd, Cmd.front());
   auto  = clang::driver::getDriverOptTable();

nridge wrote:
> 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`.
(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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436

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

[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/CompileCommands.cpp:222
+  tooling::addExpandedResponseFiles(Cmd, Command.Directory, Tokenizer, *FS);
+  tooling::addTargetAndModeForProgramName(Cmd, Cmd.front());
   auto  = clang::driver::getDriverOptTable();

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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436

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

[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin updated this revision to Diff 498038.
DmitryPolukhin added a comment.

Move standard adaptors to CommandMangler


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436

Files:
  clang-tools-extra/clangd/CompileCommands.cpp
  clang-tools-extra/clangd/CompileCommands.h
  clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
  clang-tools-extra/clangd/test/did-change-configuration-params.test
  clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
  clang/include/clang/Tooling/Tooling.h
  clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
  clang/lib/Tooling/Tooling.cpp

Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -43,6 +43,7 @@
 #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"
@@ -299,6 +300,31 @@
   }
 }
 
+void addExpandedResponseFiles(std::vector ,
+  llvm::StringRef WorkingDir,
+  llvm::cl::TokenizerCallback Tokenizer,
+  llvm::vfs::FileSystem ) {
+  bool SeenRSPFile = false;
+  llvm::SmallVector Argv;
+  Argv.reserve(CommandLine.size());
+  for (auto  : 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().setCurrentDir(WorkingDir).expandResponseFiles(Argv);
+  if (Err)
+llvm::errs() << Err;
+  // Don't assign directly, Argv aliases CommandLine.
+  std::vector ExpandedArgv(Argv.begin(), Argv.end());
+  CommandLine = std::move(ExpandedArgv);
+}
+
 } // namespace tooling
 } // namespace clang
 
@@ -684,7 +710,7 @@
 
   if (!Invocation.run())
 return nullptr;
- 
+
   assert(ASTs.size() == 1);
   return std::move(ASTs[0]);
 }
Index: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
===
--- clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
+++ clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #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"
@@ -48,28 +49,9 @@
 
 private:
   std::vector expand(std::vector Cmds) const {
-for (auto  : Cmds) {
-  bool SeenRSPFile = false;
-  llvm::SmallVector Argv;
-  Argv.reserve(Cmd.CommandLine.size());
-  for (auto  : 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 ExpandedArgv(Argv.begin(), Argv.end());
-  Cmd.CommandLine = std::move(ExpandedArgv);
-}
+for (auto  : Cmds)
+  tooling::addExpandedResponseFiles(Cmd.CommandLine, Cmd.Directory,
+Tokenizer, *FS);
 return Cmds;
   }
 
Index: clang/include/clang/Tooling/Tooling.h
===
--- clang/include/clang/Tooling/Tooling.h
+++ clang/include/clang/Tooling/Tooling.h
@@ -506,6 +506,12 @@
 void addTargetAndModeForProgramName(std::vector ,
 StringRef InvokedAs);
 
+/// Helper function that expands response files in command line.
+void addExpandedResponseFiles(std::vector ,
+  llvm::StringRef WorkingDir,
+  llvm::cl::TokenizerCallback Tokenizer,
+  llvm::vfs::FileSystem );
+
 /// Creates a \c CompilerInvocation.
 CompilerInvocation *newInvocation(DiagnosticsEngine *Diagnostics,
   ArrayRef CC1Args,
Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -49,7 +49,7 @@
   Cmd.CommandLine = {"clang++", "--", "foo.cc", "bar.cc"};
   

[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin added a comment.

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.




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1350
+->getCompileCommands(File)[0]);
 if (Old != New) {
   CDB->setCompileCommand(File, std::move(New));

kadircet wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436

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

[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

> 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.




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1350
+->getCompileCommands(File)[0]);
 if (Old != New) {
   CDB->setCompileCommand(File, std::move(New));

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436

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

[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436

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

[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

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`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436

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

[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin updated this revision to Diff 495431.
DmitryPolukhin added a comment.

Fix clang-format sources


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143436

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
  clang-tools-extra/clangd/GlobalCompilationDatabase.h
  clang-tools-extra/clangd/test/did-change-configuration-params.test
  clang/include/clang/Tooling/CompilationDatabase.h
  clang/lib/Tooling/CompilationDatabase.cpp

Index: clang/lib/Tooling/CompilationDatabase.cpp
===
--- clang/lib/Tooling/CompilationDatabase.cpp
+++ clang/lib/Tooling/CompilationDatabase.cpp
@@ -378,6 +378,11 @@
StringRef());
 }
 
+FixedCompilationDatabase::FixedCompilationDatabase(
+CompileCommand &) {
+  CompileCommands.emplace_back(CommandLine);
+}
+
 std::vector
 FixedCompilationDatabase::getCompileCommands(StringRef FilePath) const {
   std::vector Result(CompileCommands);
Index: clang/include/clang/Tooling/CompilationDatabase.h
===
--- clang/include/clang/Tooling/CompilationDatabase.h
+++ clang/include/clang/Tooling/CompilationDatabase.h
@@ -199,6 +199,9 @@
   FixedCompilationDatabase(const Twine ,
ArrayRef CommandLine);
 
+  /// Constructs a compilation data base from passed CompileCommand.
+  FixedCompilationDatabase(CompileCommand &);
+
   /// Returns the given compile command.
   ///
   /// Will always return a vector with one entry that contains the directory
Index: clang-tools-extra/clangd/test/did-change-configuration-params.test
===
--- clang-tools-extra/clangd/test/did-change-configuration-params.test
+++ clang-tools-extra/clangd/test/did-change-configuration-params.test
@@ -45,13 +45,17 @@
 # CHECK-NEXT:"uri": "file://{{.*}}/foo.c",
 # CHECK-NEXT:"version": 0
 # CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabaseChanges":{"/clangd-test/foo.c": {"workingDirectory":"/clangd-test2", "compilationCommand": ["riscv64-unknown-elf-gcc", "-c", "foo.c", "-Wall", "-Werror"]}
+#  CHECK:  "method": "textDocument/publishDiagnostics",
 #
 # ERR: ASTWorker building file {{.*}}foo.c version 0 with command
 # ERR: [{{.*}}clangd-test2]
 # ERR: clang -c -Wall -Werror {{.*}} -- {{.*}}foo.c
+# ERR: ASTWorker building file {{.*}}foo.c version 0 with command
+# ERR: [{{.*}}clangd-test2]
+# ERR: riscv64-unknown-elf-gcc --target=riscv64-unknown-elf -c -Wall -Werror {{.*}} -- {{.*}}foo.c
 ---
 {"jsonrpc":"2.0","id":5,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}
-
-
Index: clang-tools-extra/clangd/GlobalCompilationDatabase.h
===
--- clang-tools-extra/clangd/GlobalCompilationDatabase.h
+++ clang-tools-extra/clangd/GlobalCompilationDatabase.h
@@ -169,6 +169,11 @@
 SystemIncludeExtractorFn
 getSystemIncludeExtractor(llvm::ArrayRef QueryDriverGlobs);
 
+/// Creates standard set of CDB adaptors arround passed CDB.
+std::unique_ptr
+wrapCDBInExpandingAndInferingAdaptors(
+std::unique_ptr &);
+
 /// Wraps another compilation database, and supports overriding the commands
 /// using an in-memory mapping.
 class OverlayCDB : public DelegatingCDB {
Index: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
===
--- clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
+++ clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
@@ -239,20 +239,25 @@
   return {LoadResult::FoundNewData, std::move(*Buf)};
 }
 
+std::unique_ptr
+wrapCDBInExpandingAndInferingAdaptors(
+std::unique_ptr &) {
+  // 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;
+}
+
 // Adapt CDB-loading functions to a common interface for DirectoryCache::load().
 static std::unique_ptr
 parseJSON(PathRef Path, llvm::StringRef Data, std::string ) {
   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!
-// 

[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin created this revision.
DmitryPolukhin added reviewers: sammccall, ilya-biryukov.
DmitryPolukhin added projects: clang, clang-tools-extra.
Herald added subscribers: s.egerton, kadircet, arphaman, simoncook.
Herald added a project: All.
DmitryPolukhin requested review of this revision.
Herald added subscribers: pcwang-thead, MaskRay.

There is a discrepancy between how clangd processes CDB loaded from
JSON file on disk and pushed via LSP. This the same CDB pushed via
LSP protocol may not work as expected. I think there is no fundamental
difference between these CDB loaded from JSON and pushed via LSP.
This diff extracts adaptors application into a separate function and
calls it in both cases.

Test Plan: check-clang-tools


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143436

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
  clang-tools-extra/clangd/GlobalCompilationDatabase.h
  clang-tools-extra/clangd/test/did-change-configuration-params.test
  clang/include/clang/Tooling/CompilationDatabase.h
  clang/lib/Tooling/CompilationDatabase.cpp

Index: clang/lib/Tooling/CompilationDatabase.cpp
===
--- clang/lib/Tooling/CompilationDatabase.cpp
+++ clang/lib/Tooling/CompilationDatabase.cpp
@@ -378,6 +378,10 @@
StringRef());
 }
 
+FixedCompilationDatabase::FixedCompilationDatabase(CompileCommand&& CommandLine) {
+  CompileCommands.emplace_back(CommandLine);
+}
+
 std::vector
 FixedCompilationDatabase::getCompileCommands(StringRef FilePath) const {
   std::vector Result(CompileCommands);
Index: clang/include/clang/Tooling/CompilationDatabase.h
===
--- clang/include/clang/Tooling/CompilationDatabase.h
+++ clang/include/clang/Tooling/CompilationDatabase.h
@@ -199,6 +199,9 @@
   FixedCompilationDatabase(const Twine ,
ArrayRef CommandLine);
 
+  /// Constructs a compilation data base from passed CompileCommand.
+  FixedCompilationDatabase(CompileCommand&& CommandLine);
+
   /// Returns the given compile command.
   ///
   /// Will always return a vector with one entry that contains the directory
Index: clang-tools-extra/clangd/test/did-change-configuration-params.test
===
--- clang-tools-extra/clangd/test/did-change-configuration-params.test
+++ clang-tools-extra/clangd/test/did-change-configuration-params.test
@@ -45,13 +45,17 @@
 # CHECK-NEXT:"uri": "file://{{.*}}/foo.c",
 # CHECK-NEXT:"version": 0
 # CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabaseChanges":{"/clangd-test/foo.c": {"workingDirectory":"/clangd-test2", "compilationCommand": ["riscv64-unknown-elf-gcc", "-c", "foo.c", "-Wall", "-Werror"]}
+#  CHECK:  "method": "textDocument/publishDiagnostics",
 #
 # ERR: ASTWorker building file {{.*}}foo.c version 0 with command
 # ERR: [{{.*}}clangd-test2]
 # ERR: clang -c -Wall -Werror {{.*}} -- {{.*}}foo.c
+# ERR: ASTWorker building file {{.*}}foo.c version 0 with command
+# ERR: [{{.*}}clangd-test2]
+# ERR: riscv64-unknown-elf-gcc --target=riscv64-unknown-elf -c -Wall -Werror {{.*}} -- {{.*}}foo.c
 ---
 {"jsonrpc":"2.0","id":5,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}
-
-
Index: clang-tools-extra/clangd/GlobalCompilationDatabase.h
===
--- clang-tools-extra/clangd/GlobalCompilationDatabase.h
+++ clang-tools-extra/clangd/GlobalCompilationDatabase.h
@@ -169,6 +169,11 @@
 SystemIncludeExtractorFn
 getSystemIncludeExtractor(llvm::ArrayRef QueryDriverGlobs);
 
+/// Creates standard set of CDB adaptors arround passed CDB.
+std::unique_ptr
+wrapCDBInExpandingAndInferingAdaptors(
+std::unique_ptr &);
+
 /// Wraps another compilation database, and supports overriding the commands
 /// using an in-memory mapping.
 class OverlayCDB : public DelegatingCDB {
Index: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
===
--- clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
+++ clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
@@ -239,20 +239,25 @@
   return {LoadResult::FoundNewData, std::move(*Buf)};
 }
 
+std::unique_ptr
+wrapCDBInExpandingAndInferingAdaptors(
+std::unique_ptr &) {
+  // 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