[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D88338#2307976 , @sammccall wrote:

> Sorry @rsmith @hoy, I've replaced the test with one without such dependencies 
> in bc18d8d9b705d31a94c51900c8b18e4feaf9c7fb 
> .

Thanks for the quick turnaround!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88338

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

[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Sorry @rsmith @hoy, I've replaced the test with one without such dependencies 
in bc18d8d9b705d31a94c51900c8b18e4feaf9c7fb 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88338

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

[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

[Hongtao Yu via Phabricator via cfe-commits]

hoy added inline comments.



Comment at: clang-tools-extra/clangd/tool/Check.cpp:243
+  size_t N = 50;
+  auto xxx = std::string(N, 'x');
+)cpp";

Hi, I'm seeing an error with this "tweak: ExpandAutoType ==> FAIL: Could not 
deduce type for 'auto' type". It's probably due to that the std string header 
was not available when running `check.test`, thus the test failed. Is it 
expected that the header is available when test is run? Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88338

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

[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang-tools-extra/clangd/test/check.test:1
+# RUN: clangd -log=verbose -check 2>&1 | FileCheck -strict-whitespace %s
+

This test implicitly parses a source file that `#include`s standard library 
headers, and fails if those headers aren't available; this causes the test to 
fail in some build environments. Would it be possible to make this test work in 
freestanding environments?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88338

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

[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, I think this fails whenever `compile_commands.json` doesn't exist in 
the tree or under `build`.
(Which of course it does in my local checkout...). Reverted, probably need to 
rename/copy the test file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88338

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

[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Might break tests: http://45.33.8.238/linux/29238/step_9.txt


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88338

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

[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

[Sam McCall via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rG79fbcbff4173: [clangd] clangd --check: standalone diagnosis 
of common problems (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D88338?vs=295290=295558#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88338

Files:
  clang-tools-extra/clangd/test/check-fail.test
  clang-tools-extra/clangd/test/check.test
  clang-tools-extra/clangd/tool/CMakeLists.txt
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -47,6 +47,11 @@
 
 namespace clang {
 namespace clangd {
+
+// Implemented in Check.cpp.
+bool check(const llvm::StringRef File, const ThreadsafeFS ,
+   const ClangdLSPServer::Options );
+
 namespace {
 
 using llvm::cl::cat;
@@ -57,6 +62,7 @@
 using llvm::cl::list;
 using llvm::cl::opt;
 using llvm::cl::OptionCategory;
+using llvm::cl::ValueOptional;
 using llvm::cl::values;
 
 // All flags must be placed in a category, or they will be shown neither in
@@ -354,6 +360,16 @@
 Hidden,
 };
 
+opt CheckFile{
+"check",
+cat(Misc),
+desc("Parse one file in isolation instead of acting as a language server. "
+ "Useful to investigate/reproduce crashes or configuration problems. "
+ "With --check=, attempts to parse a particular file."),
+init(""),
+ValueOptional,
+};
+
 enum PCHStorageFlag { Disk, Memory };
 opt PCHStorage{
 "pch-storage",
@@ -541,7 +557,8 @@
 
 enum class ErrorResultCode : int {
   NoShutdownRequest = 1,
-  CantRunAsXPCService = 2
+  CantRunAsXPCService = 2,
+  CheckFailed = 3
 };
 
 int main(int argc, char *argv[]) {
@@ -646,7 +663,8 @@
   // If a user ran `clangd` in a terminal without redirecting anything,
   // it's somewhat likely they're confused about how to use clangd.
   // Show them the help overview, which explains.
-  if (llvm::outs().is_displayed() && llvm::errs().is_displayed())
+  if (llvm::outs().is_displayed() && llvm::errs().is_displayed() &&
+  !CheckFile.getNumOccurrences())
 llvm::errs() << Overview << "\n";
   // Use buffered stream to stderr (we still flush each log message). Unbuffered
   // stream can cause significant (non-deterministic) latency for the logger.
@@ -825,6 +843,15 @@
   // Shall we allow to customize the file limit?
   Opts.Rename.AllowCrossFile = CrossFileRename;
 
+  if (CheckFile.getNumOccurrences()) {
+llvm::SmallString<256> Path;
+llvm::sys::fs::real_path(CheckFile, Path, /*expand_tilde=*/true);
+log("Entering check mode (no LSP server)");
+return check(Path, TFS, Opts)
+   ? 0
+   : static_cast(ErrorResultCode::CheckFailed);
+  }
+
   // Initialize and run ClangdLSPServer.
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();
@@ -835,7 +862,7 @@
 TransportLayer = newXPCTransport();
 #else
 llvm::errs() << "This clangd binary wasn't built with XPC support.\n";
-return (int)ErrorResultCode::CantRunAsXPCService;
+return static_cast(ErrorResultCode::CantRunAsXPCService);
 #endif
   } else {
 log("Starting LSP over stdin/stdout");
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -0,0 +1,258 @@
+//===--- Check.cpp - clangd self-diagnostics --===//
+//
+// 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
+//
+//===--===//
+//
+// Many basic problems can occur processing a file in clangd, e.g.:
+//  - system includes are not found
+//  - crash when indexing its AST
+// clangd --check provides a simplified, isolated way to reproduce these,
+// with no editor, LSP, threads, background indexing etc to contend with.
+//
+// One important use case is gathering information for bug reports.
+// Another is reproducing crashes, and checking which setting prevent them.
+//
+// It simulates opening a file (determining compile command, parsing, indexing)
+// and then running features at many locations.
+//
+// Currently it adds some basic logging of progress and results.
+// We should consider extending it to also recognize common symptoms and
+// recommend solutions (e.g. standard library installation issues).
+//

[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks LGTM!




Comment at: clang-tools-extra/clangd/tool/Check.cpp:107
+
+Cmd = CDB->getFallbackCommand(File);
+if (auto TrueCmd = CDB->getCompileCommand(File)) {

already done in the else statement.



Comment at: clang-tools-extra/clangd/tool/Check.cpp:125
+Inputs.Opts.ClangTidyOpts =
+Opts.GetClangTidyOptions(*TFS.view(llvm::None), File);
+log("Parsing command...");

sammccall wrote:
> kadircet wrote:
> > IIRC, option providers don't really log anything about success/failure of 
> > parsing the config file. what's the point of having this exactly?
> This is needed in order to run the correct clang-tidy checks.
ah sorry, i forgot `Inputs` was a member



Comment at: clang-tools-extra/clangd/tool/Check.cpp:131
+showErrors(InvocationDiags);
+log("cc1 args are: {0}", llvm::join(CC1Args, " "));
+if (!Invocation) {

sammccall wrote:
> kadircet wrote:
> > maybe we should also note that this doesn't reflect the final result. as we 
> > turn off pchs or dependency file outputting related flags afterwards.
> Sure, and we also run clang-tidy, and fiddle with the preamble, and skip 
> function bodies...
> 
> I think a disclaimer that "running clangd is not equivalent to running clang 
> " would be somewhat confusing, as I'm not sure that's a thing that 
> anyone would expect to be true.
> 
> I expect the cc1 args to be useless to anyone not familiar with clangd 
> internals, but they're kind of important to include. Reworded the log message 
> a bit, is this enough?
SG, and actually thinking again, we are only making changes in the positive 
direction for clangd. i.e. there shouldn't be an instance in which this cc1 
works, but clangd fails.



Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:52
+// Implemented in Check.cpp.
+bool check(const llvm::StringRef File, const ThreadsafeFS ,
+   llvm::Optional CompileCommandsPath,

sammccall wrote:
> kadircet wrote:
> > why not have this in `Check.h` ?
> A header seems a bit out of place in tool/ and it doesn't seem necessary, 
> being one function with a simple signature and no unit tests.
> 
> We'll get a linker error if we ever get this wrong, right?
yeah, it just felt uncommon, i don't think we can mess this up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88338

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

[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 295290.
sammccall marked 6 inline comments as done.
sammccall added a comment.

address (some of) comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88338

Files:
  clang-tools-extra/clangd/test/check-fail.test
  clang-tools-extra/clangd/test/check.test
  clang-tools-extra/clangd/tool/CMakeLists.txt
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -47,6 +47,11 @@
 
 namespace clang {
 namespace clangd {
+
+// Implemented in Check.cpp.
+bool check(const llvm::StringRef File, const ThreadsafeFS ,
+   const ClangdLSPServer::Options );
+
 namespace {
 
 using llvm::cl::cat;
@@ -57,6 +62,7 @@
 using llvm::cl::list;
 using llvm::cl::opt;
 using llvm::cl::OptionCategory;
+using llvm::cl::ValueOptional;
 using llvm::cl::values;
 
 // All flags must be placed in a category, or they will be shown neither in
@@ -354,6 +360,16 @@
 Hidden,
 };
 
+opt CheckFile{
+"check",
+cat(Misc),
+desc("Parse one file in isolation instead of acting as a language server. "
+ "Useful to investigate/reproduce crashes or configuration problems. "
+ "With --check=, attempts to parse a particular file."),
+init(""),
+ValueOptional,
+};
+
 enum PCHStorageFlag { Disk, Memory };
 opt PCHStorage{
 "pch-storage",
@@ -541,7 +557,8 @@
 
 enum class ErrorResultCode : int {
   NoShutdownRequest = 1,
-  CantRunAsXPCService = 2
+  CantRunAsXPCService = 2,
+  CheckFailed = 3
 };
 
 int main(int argc, char *argv[]) {
@@ -646,7 +663,8 @@
   // If a user ran `clangd` in a terminal without redirecting anything,
   // it's somewhat likely they're confused about how to use clangd.
   // Show them the help overview, which explains.
-  if (llvm::outs().is_displayed() && llvm::errs().is_displayed())
+  if (llvm::outs().is_displayed() && llvm::errs().is_displayed() &&
+  !CheckFile.getNumOccurrences())
 llvm::errs() << Overview << "\n";
   // Use buffered stream to stderr (we still flush each log message). Unbuffered
   // stream can cause significant (non-deterministic) latency for the logger.
@@ -825,6 +843,15 @@
   // Shall we allow to customize the file limit?
   Opts.Rename.AllowCrossFile = CrossFileRename;
 
+  if (CheckFile.getNumOccurrences()) {
+llvm::SmallString<256> Path;
+llvm::sys::fs::real_path(CheckFile, Path, /*expand_tilde=*/true);
+log("Entering check mode (no LSP server)");
+return check(Path, TFS, Opts)
+   ? 0
+   : static_cast(ErrorResultCode::CheckFailed);
+  }
+
   // Initialize and run ClangdLSPServer.
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();
@@ -835,7 +862,7 @@
 TransportLayer = newXPCTransport();
 #else
 llvm::errs() << "This clangd binary wasn't built with XPC support.\n";
-return (int)ErrorResultCode::CantRunAsXPCService;
+return static_cast(ErrorResultCode::CantRunAsXPCService);
 #endif
   } else {
 log("Starting LSP over stdin/stdout");
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -0,0 +1,259 @@
+//===--- Check.cpp - clangd self-diagnostics --===//
+//
+// 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
+//
+//===--===//
+//
+// Many basic problems can occur processing a file in clangd, e.g.:
+//  - system includes are not found
+//  - crash when indexing its AST
+// clangd --check provides a simplified, isolated way to reproduce these,
+// with no editor, LSP, threads, background indexing etc to contend with.
+//
+// One important use case is gathering information for bug reports.
+// Another is reproducing crashes, and checking which setting prevent them.
+//
+// It simulates opening a file (determining compile command, parsing, indexing)
+// and then running features at many locations.
+//
+// Currently it adds some basic logging of progress and results.
+// We should consider extending it to also recognize common symptoms and
+// recommend solutions (e.g. standard library installation issues).
+//
+//===--===//
+
+#include "ClangdLSPServer.h"
+#include "CodeComplete.h"
+#include "GlobalCompilationDatabase.h"
+#include "Hover.h"
+#include "ParsedAST.h"
+#include "Preamble.h"
+#include "SourceCode.h"
+#include "XRefs.h"
+#include 

[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/tool/Check.cpp:87
+auto Mangler = CommandMangler::detect();
+// Check for missing resource dir?
+if (Opts.ResourceDir)

kadircet wrote:
> i agree, this would help identifying the case of "clangd binary has been 
> copied to some place without the necessary lib directory".
> 
> but i think we should check for the final `-resource-dir` in the compile 
> command below. as user's compile flags can override whatever default clangd 
> comes up with.
Moved FIXME to the right place.



Comment at: clang-tools-extra/clangd/tool/Check.cpp:125
+Inputs.Opts.ClangTidyOpts =
+Opts.GetClangTidyOptions(*TFS.view(llvm::None), File);
+log("Parsing command...");

kadircet wrote:
> IIRC, option providers don't really log anything about success/failure of 
> parsing the config file. what's the point of having this exactly?
This is needed in order to run the correct clang-tidy checks.



Comment at: clang-tools-extra/clangd/tool/Check.cpp:126
+Opts.GetClangTidyOptions(*TFS.view(llvm::None), File);
+log("Parsing command...");
+Invocation =

kadircet wrote:
> it is clear that we've crashed while parsing compile commands if we don't see 
> `cc1 args are` in the output. so maybe drop this one?
I don't think that's clear - you can find it out by reading the code, but I 
expect people to be running this who aren't reading the code. (They won't be 
able to work out all the details, but they tend to be interested in the broad 
strokes of what's going on).

So generally I've tried to include log statements before each major step.



Comment at: clang-tools-extra/clangd/tool/Check.cpp:131
+showErrors(InvocationDiags);
+log("cc1 args are: {0}", llvm::join(CC1Args, " "));
+if (!Invocation) {

kadircet wrote:
> maybe we should also note that this doesn't reflect the final result. as we 
> turn off pchs or dependency file outputting related flags afterwards.
Sure, and we also run clang-tidy, and fiddle with the preamble, and skip 
function bodies...

I think a disclaimer that "running clangd is not equivalent to running clang 
" would be somewhat confusing, as I'm not sure that's a thing that 
anyone would expect to be true.

I expect the cc1 args to be useless to anyone not familiar with clangd 
internals, but they're kind of important to include. Reworded the log message a 
bit, is this enough?



Comment at: clang-tools-extra/clangd/tool/Check.cpp:202
+  }
+  unsigned Definitions = locateSymbolAt(*AST, Pos, ).size();
+  vlog("definition: {0}", Definitions);

kadircet wrote:
> maybe we could print errors if the following has no results for "identifiers" 
> ?
There are lots of ways go-to-def can yield no results (e.g. macros, #ifdef-'d 
sections, templated code we can't resolve)



Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:52
+// Implemented in Check.cpp.
+bool check(const llvm::StringRef File, const ThreadsafeFS ,
+   llvm::Optional CompileCommandsPath,

kadircet wrote:
> why not have this in `Check.h` ?
A header seems a bit out of place in tool/ and it doesn't seem necessary, being 
one function with a simple signature and no unit tests.

We'll get a linker error if we ever get this wrong, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88338

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

[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks! this mostly looks good, as discussed offline I believe having an infra 
that we can improve over time is better than not having anything until we've 
got the "perfect" solution.

i just got a couple of questions about some pieces, and some nits.




Comment at: clang-tools-extra/clangd/tool/Check.cpp:57
+// Nonfatal failures are logged and tracked in ErrCount.
+class Checker {
+  // from constructor

put into anon namespace?



Comment at: clang-tools-extra/clangd/tool/Check.cpp:87
+auto Mangler = CommandMangler::detect();
+// Check for missing resource dir?
+if (Opts.ResourceDir)

i agree, this would help identifying the case of "clangd binary has been copied 
to some place without the necessary lib directory".

but i think we should check for the final `-resource-dir` in the compile 
command below. as user's compile flags can override whatever default clangd 
comes up with.



Comment at: clang-tools-extra/clangd/tool/Check.cpp:95
+Cmd = CDB->getFallbackCommand(File);
+log("Generic fallback command is: {0}", llvm::join(Cmd.CommandLine, " "));
+if (auto TrueCmd = CDB->getCompileCommand(File)) {

we don't have any custom fallback commands, what's the point of printing this 
always?



Comment at: clang-tools-extra/clangd/tool/Check.cpp:125
+Inputs.Opts.ClangTidyOpts =
+Opts.GetClangTidyOptions(*TFS.view(llvm::None), File);
+log("Parsing command...");

IIRC, option providers don't really log anything about success/failure of 
parsing the config file. what's the point of having this exactly?



Comment at: clang-tools-extra/clangd/tool/Check.cpp:126
+Opts.GetClangTidyOptions(*TFS.view(llvm::None), File);
+log("Parsing command...");
+Invocation =

it is clear that we've crashed while parsing compile commands if we don't see 
`cc1 args are` in the output. so maybe drop this one?



Comment at: clang-tools-extra/clangd/tool/Check.cpp:131
+showErrors(InvocationDiags);
+log("cc1 args are: {0}", llvm::join(CC1Args, " "));
+if (!Invocation) {

maybe we should also note that this doesn't reflect the final result. as we 
turn off pchs or dependency file outputting related flags afterwards.



Comment at: clang-tools-extra/clangd/tool/Check.cpp:137
+
+Style = getFormatStyleForFile(File, Inputs.Contents, TFS);
+

this seems ... surprising :D but I also don't have suggestion for a better 
place.



Comment at: clang-tools-extra/clangd/tool/Check.cpp:202
+  }
+  unsigned Definitions = locateSymbolAt(*AST, Pos, ).size();
+  vlog("definition: {0}", Definitions);

maybe we could print errors if the following has no results for "identifiers" ?



Comment at: clang-tools-extra/clangd/tool/Check.cpp:214
+  // Print (and count) the error-level diagnostics (warnings are ignored).
+  void showErrors(llvm::ArrayRef Diags) {
+for (const auto  : Diags) {

nit: maybe make this a free function and `return ErrCount` ?



Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:52
+// Implemented in Check.cpp.
+bool check(const llvm::StringRef File, const ThreadsafeFS ,
+   llvm::Optional CompileCommandsPath,

why not have this in `Check.h` ?



Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:833
+   ? 0
+   : (int)ErrorResultCode::CheckFailed;
+  }

nit: `static_cast(Err..)`

maybe do the same for line 846 while here. (line 872 already does that)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88338

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

[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, arphaman, mgorny.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

This is a tool to simply parse a file as clangd would, and run some
common features (code actions, go-to-definition, hover) in an attempt to
trigger or reproduce crashes, error diagnostics, etc.

This is easier and more predictable than loading the file in clangd, because:

- there's no editor/plugin variation to worry about
- there's no accidental variation of user behavior or other extraneous requests
- we trigger features at every token, rather than guessing
- everything is synchronoous, logs are easier to reason about
- it's easier to (get users to) capture logs when running on the command-line

This is a fairly lightweight variant of this idea.
We could do a lot more with it, and maybe we should.
But I can't in the near future, and experience will tell us if we made
the right tradeoffs and if it's worth investing further.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88338

Files:
  clang-tools-extra/clangd/test/check-fail.test
  clang-tools-extra/clangd/test/check.test
  clang-tools-extra/clangd/tool/CMakeLists.txt
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -47,6 +47,12 @@
 
 namespace clang {
 namespace clangd {
+
+// Implemented in Check.cpp.
+bool check(const llvm::StringRef File, const ThreadsafeFS ,
+   llvm::Optional CompileCommandsPath,
+   const ClangdServer::Options );
+
 namespace {
 
 using llvm::cl::cat;
@@ -57,6 +63,7 @@
 using llvm::cl::list;
 using llvm::cl::opt;
 using llvm::cl::OptionCategory;
+using llvm::cl::ValueOptional;
 using llvm::cl::values;
 
 // All flags must be placed in a category, or they will be shown neither in
@@ -334,6 +341,16 @@
 Hidden,
 };
 
+opt CheckFile{
+"check",
+cat(Misc),
+desc("Parse one file in isolation instead of acting as a language server. "
+ "Useful to investigate/reproduce crashes or configuration problems. "
+ "With --check=, attempts to parse a particular file."),
+init(""),
+ValueOptional,
+};
+
 enum PCHStorageFlag { Disk, Memory };
 opt PCHStorage{
 "pch-storage",
@@ -521,7 +538,8 @@
 
 enum class ErrorResultCode : int {
   NoShutdownRequest = 1,
-  CantRunAsXPCService = 2
+  CantRunAsXPCService = 2,
+  CheckFailed = 3
 };
 
 int main(int argc, char *argv[]) {
@@ -626,7 +644,8 @@
   // If a user ran `clangd` in a terminal without redirecting anything,
   // it's somewhat likely they're confused about how to use clangd.
   // Show them the help overview, which explains.
-  if (llvm::outs().is_displayed() && llvm::errs().is_displayed())
+  if (llvm::outs().is_displayed() && llvm::errs().is_displayed() &&
+  !CheckFile.getNumOccurrences())
 llvm::errs() << Overview << "\n";
   // Use buffered stream to stderr (we still flush each log message). Unbuffered
   // stream can cause significant (non-deterministic) latency for the logger.
@@ -805,6 +824,15 @@
   // Shall we allow to customize the file limit?
   RenameOpts.AllowCrossFile = CrossFileRename;
 
+  if (CheckFile.getNumOccurrences()) {
+llvm::SmallString<256> Path;
+llvm::sys::fs::real_path(CheckFile, Path, /*expand_tilde=*/true);
+log("Entering check mode (no LSP server)");
+return check(Path, TFS, CompileCommandsDirPath, Opts)
+   ? 0
+   : (int)ErrorResultCode::CheckFailed;
+  }
+
   // Initialize and run ClangdLSPServer.
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -0,0 +1,254 @@
+//===--- Check.cpp - clangd self-diagnostics --===//
+//
+// 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
+//
+//===--===//
+//
+// Many basic problems can occur processing a file in clangd, e.g.:
+//  - system includes are not found
+//  - crash when indexing its AST
+// clangd --check provides a simplified, isolated way to reproduce these,
+// with no editor, LSP, threads, background indexing etc to contend with.
+//
+// One important use case is gathering information for bug reports.
+// Another is reproducing crashes, and checking which setting prevent them.
+//
+// It simulates opening a