[PATCH] D104056: [clangd][nfc] Show more information in logs when compiler instance prepare fails

[Aleksandr Platonov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa62579fc008e: [clangd][nfc] Show more information in logs 
when compiler instance prepare fails (authored by ArcsinX).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104056

Files:
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -390,8 +390,8 @@
 SerializedDeclsCollector.takeMacros(), std::move(StatCache),
 SerializedDeclsCollector.takeCanonicalIncludes());
   } else {
-elog("Could not build a preamble for file {0} version {1}", FileName,
- Inputs.Version);
+elog("Could not build a preamble for file {0} version {1}: {2}", FileName,
+ Inputs.Version, BuiltPreamble.getError().message());
 return nullptr;
   }
 }
Index: clang-tools-extra/clangd/ParsedAST.cpp
===
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -289,8 +289,15 @@
   std::move(CI), PreamblePCH,
   llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents, Filename), VFS,
   ASTDiags);
-  if (!Clang)
+  if (!Clang) {
+// The last diagnostic contains information about the reason of this
+// failure.
+std::vector Diags(ASTDiags.take());
+elog("Failed to prepare a compiler instance: {0}",
+ !Diags.empty() ? static_cast(Diags.back()).Message
+: "unknown error");
 return None;
+  }
 
   auto Action = std::make_unique();
   const FrontendInputFile  = Clang->getFrontendOpts().Inputs[0];


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -390,8 +390,8 @@
 SerializedDeclsCollector.takeMacros(), std::move(StatCache),
 SerializedDeclsCollector.takeCanonicalIncludes());
   } else {
-elog("Could not build a preamble for file {0} version {1}", FileName,
- Inputs.Version);
+elog("Could not build a preamble for file {0} version {1}: {2}", FileName,
+ Inputs.Version, BuiltPreamble.getError().message());
 return nullptr;
   }
 }
Index: clang-tools-extra/clangd/ParsedAST.cpp
===
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -289,8 +289,15 @@
   std::move(CI), PreamblePCH,
   llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents, Filename), VFS,
   ASTDiags);
-  if (!Clang)
+  if (!Clang) {
+// The last diagnostic contains information about the reason of this
+// failure.
+std::vector Diags(ASTDiags.take());
+elog("Failed to prepare a compiler instance: {0}",
+ !Diags.empty() ? static_cast(Diags.back()).Message
+: "unknown error");
 return None;
+  }
 
   auto Action = std::make_unique();
   const FrontendInputFile  = Clang->getFrontendOpts().Inputs[0];
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104056: [clangd][nfc] Show more information in logs when compiler instance prepare fails

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX updated this revision to Diff 355389.
ArcsinX added a comment.

Check ASTDiags.take() for empty instead of ASTDiags.getNumErrors() value check


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104056

Files:
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -390,8 +390,8 @@
 SerializedDeclsCollector.takeMacros(), std::move(StatCache),
 SerializedDeclsCollector.takeCanonicalIncludes());
   } else {
-elog("Could not build a preamble for file {0} version {1}", FileName,
- Inputs.Version);
+elog("Could not build a preamble for file {0} version {1}: {2}", FileName,
+ Inputs.Version, BuiltPreamble.getError().message());
 return nullptr;
   }
 }
Index: clang-tools-extra/clangd/ParsedAST.cpp
===
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -289,8 +289,15 @@
   std::move(CI), PreamblePCH,
   llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents, Filename), VFS,
   ASTDiags);
-  if (!Clang)
+  if (!Clang) {
+// The last diagnostic contains information about the reason of this
+// failure.
+std::vector Diags(ASTDiags.take());
+elog("Failed to prepare a compiler instance: {0}",
+ !Diags.empty() ? static_cast(Diags.back()).Message
+: "unknown error");
 return None;
+  }
 
   auto Action = std::make_unique();
   const FrontendInputFile  = Clang->getFrontendOpts().Inputs[0];


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -390,8 +390,8 @@
 SerializedDeclsCollector.takeMacros(), std::move(StatCache),
 SerializedDeclsCollector.takeCanonicalIncludes());
   } else {
-elog("Could not build a preamble for file {0} version {1}", FileName,
- Inputs.Version);
+elog("Could not build a preamble for file {0} version {1}: {2}", FileName,
+ Inputs.Version, BuiltPreamble.getError().message());
 return nullptr;
   }
 }
Index: clang-tools-extra/clangd/ParsedAST.cpp
===
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -289,8 +289,15 @@
   std::move(CI), PreamblePCH,
   llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents, Filename), VFS,
   ASTDiags);
-  if (!Clang)
+  if (!Clang) {
+// The last diagnostic contains information about the reason of this
+// failure.
+std::vector Diags(ASTDiags.take());
+elog("Failed to prepare a compiler instance: {0}",
+ !Diags.empty() ? static_cast(Diags.back()).Message
+: "unknown error");
 return None;
+  }
 
   auto Action = std::make_unique();
   const FrontendInputFile  = Clang->getFrontendOpts().Inputs[0];
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104056: [clangd][nfc] Show more information in logs when compiler instance prepare fails

[Sam McCall via Phabricator via cfe-commits]

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

Thanks, LGTM

Sorry about the delay - we got sidetracked into some ideas about whether these 
errors should actually be surfaced to the user as diagnostics instead. But that 
shouldn't block this patch.




Comment at: clang-tools-extra/clangd/ParsedAST.cpp:296
+elog("Failed to prepare a compiler instance: {0}",
+ ASTDiags.getNumErrors()
+ ? static_cast(ASTDiags.take().back()).Message

there's a bunch of logic that determines which diags we actually end up storing.

I'd suggest always calling take() and testing whether the result is empty, 
rather than using getNumErrors() which bypasses all our logic.

Luckily, we do (always?) end up storing the relevant diags here, assuming 
they're errors without a source location attached.
However I don't think we should hard-code that assumption here.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104056

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

[PATCH] D104056: [clangd][nfc] Show more information in logs when compiler instance prepare fails

[Aleksandr Platonov via Phabricator via cfe-commits]

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

Without this patch clangd silently process compiler instance prepare failure 
and only LSP errors "Invalid AST" could be found in logs.
E.g. the reason of the problem https://github.com/clangd/clangd/issues/734 is 
impossible to understand without verbose logs or with disabled background index.
This patch adds more information into logs to help understand the reason of 
such failures.

Logs without this patch:

  E[...] Could not build a preamble for file test version 1

Logs with this patch:

  E[...] Could not build a preamble for file test.cpp version 1: 
CreateTargetInfo() return null
  ..
  E[...] Failed to prepare a compiler instance: unknown target ABI 'lp64'


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104056

Files:
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -390,8 +390,8 @@
 SerializedDeclsCollector.takeMacros(), std::move(StatCache),
 SerializedDeclsCollector.takeCanonicalIncludes());
   } else {
-elog("Could not build a preamble for file {0} version {1}", FileName,
- Inputs.Version);
+elog("Could not build a preamble for file {0} version {1}: {2}", FileName,
+ Inputs.Version, BuiltPreamble.getError().message());
 return nullptr;
   }
 }
Index: clang-tools-extra/clangd/ParsedAST.cpp
===
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -289,8 +289,15 @@
   std::move(CI), PreamblePCH,
   llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents, Filename), VFS,
   ASTDiags);
-  if (!Clang)
+  if (!Clang) {
+// The last diagnostic contains information about the reason of this
+// failure.
+elog("Failed to prepare a compiler instance: {0}",
+ ASTDiags.getNumErrors()
+ ? static_cast(ASTDiags.take().back()).Message
+ : "unknown error");
 return None;
+  }
 
   auto Action = std::make_unique();
   const FrontendInputFile  = Clang->getFrontendOpts().Inputs[0];


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -390,8 +390,8 @@
 SerializedDeclsCollector.takeMacros(), std::move(StatCache),
 SerializedDeclsCollector.takeCanonicalIncludes());
   } else {
-elog("Could not build a preamble for file {0} version {1}", FileName,
- Inputs.Version);
+elog("Could not build a preamble for file {0} version {1}: {2}", FileName,
+ Inputs.Version, BuiltPreamble.getError().message());
 return nullptr;
   }
 }
Index: clang-tools-extra/clangd/ParsedAST.cpp
===
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -289,8 +289,15 @@
   std::move(CI), PreamblePCH,
   llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents, Filename), VFS,
   ASTDiags);
-  if (!Clang)
+  if (!Clang) {
+// The last diagnostic contains information about the reason of this
+// failure.
+elog("Failed to prepare a compiler instance: {0}",
+ ASTDiags.getNumErrors()
+ ? static_cast(ASTDiags.take().back()).Message
+ : "unknown error");
 return None;
+  }
 
   auto Action = std::make_unique();
   const FrontendInputFile  = Clang->getFrontendOpts().Inputs[0];
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits