[clangd][tests] Add exit(EXIT_FAILURE) in case of JSON parsing failure in TestMode

[jkorous]

Summary

This is meant to cause a visible fail when clangd gets invalid JSON payload in -lit-test mode.

Based on discussion in https://reviews.llvm.org/D50641 (and http://lists.llvm.org/pipermail/clangd-dev/2018-August/000072.html).

[arphaman]

I think propagating the 'test' yes/no value is not the best way to describe the intention of this change.
Our intention is to exit from the LSP server on JSON error. One way to describe this intention better is to propagate a boolean 'exitOnJSONError' value.
Furthermore, If you think about the '-llvm-lit' flag itself you'll see that it acts an abbreviation for these 3 options: -input-style=delimited -pretty -run-synchronously. I think that it would be valuable to follow this intention and to add a new option (e.g. -exit-on-json-error) that will then be set when -llvm-lit is set.
WDYT?

[ilya-biryukov]

+1 to having a separate option for that. More generally, wouldn't we want to exit on more kinds errors in the future?
JSON parsing errors is something that popped up recently, but the option only for that looks too narrow. We might end up wanting more options like that in the future, e.g. -exit-on-json-dispatch-error, -exit-on-parse-error, ...

How about a more generic option that would force clangd to have non-zero error code whenever even a single error was emitted via elog()?
I've checked the output of ./bin/llvm-lit -avv ../tools/clang/tools/extra/test/clangd/ | grep '\(^E\|PASS\)' and there seems to be a few things that emit errors spuriously and should be fixed for that to work:

1. E[10:19:11.985] Failed to get status for RootPath clangd: No such file or directory.

Could be fixed by specifying a valid rootPath in all clangd tests.

2. E[10:19:12.013] Could not build a preamble for file /clangd-test/main.cpp.

IIUC, this happens on empty preambles, the fix is to not emit an error in that case. In general, whenever preamble is not built, this is probably due to changes in some headers and that case should be considered valid input for clangd, so arguably this shouldn't be an error in the first place.-

3. Some tests actually test for invalid inputs, those should probably expect an error code from clangd that runs into those errors.

That doesn't look like a long list, so it shouldn't be too hard. What are your thoughts on a more generic option like that?

JSONRPCDispatcher.cpp

366

Alternative would be to run input parsing to completion, propagate if any error was encountered to main and exit with a predefined error code in that case.
Exiting prematurely might hide some important errors that we would otherwise catch even before seeing an error code from clangd, e.g. infinite loops in the input handling on invalid inputs, etc.

[jkorous]

Oh, I thought that what everyone wanted was test-specific behaviour. I like both approaches you propose much more!
If we go for the generic option we can effectively start "checking stderr" in tests by using the flag. That would be nice.

Just to be sure we are all on the same page:

1. Do we want to exit right after elog() call?
2. Do we consider calling exit() in elog() smart or hacky?

Based on quick grepping if we'd like to propagate errors probably the biggest challenge would be ASTWorker.

ClangdLSPServer.h

ClangdLSPServer(JSONOutput &Out, const clangd::CodeCompleteOptions &CCOpts,
                   bool ShouldUseInMemoryCDB, const ClangdServer::Options &Opts);
 
   /// Run LSP server loop, receiving input for it from \p In. \p In must be
   /// opened in binary mode. Output will be written using Out variable passed to
   /// class constructor. This method must not be executed more than once for
   /// each instance of ClangdLSPServer.
   ///
   /// \return Whether we received a 'shutdown' request before an 'exit' request.
-  bool run(std::FILE *In,
+  bool run(std::FILE *In, bool TestMode,
            JSONStreamStyle InputStyle = JSONStreamStyle::Standard);
 
 private:
   // Implement DiagnosticsConsumer.
   void onDiagnosticsReady(PathRef File, std::vector<Diag> Diagnostics) override;
 
   // Implement ProtocolCallbacks.
   void onInitialize(InitializeParams &Params) override;

ClangdLSPServer.cpp

ClangdLSPServer::ClangdLSPServer(JSONOutput &Out,
                                  bool ShouldUseInMemoryCDB,
                                  const ClangdServer::Options &Opts)
     : Out(Out), CDB(ShouldUseInMemoryCDB ? CompilationDB::makeInMemory()
                                          : CompilationDB::makeDirectoryBased(
                                                std::move(CompileCommandsDir))),
       CCOpts(CCOpts), SupportedSymbolKinds(defaultSymbolKinds()),
       Server(CDB.getCDB(), FSProvider, /*DiagConsumer=*/*this, Opts) {}
 
-bool ClangdLSPServer::run(std::FILE *In, JSONStreamStyle InputStyle) {
+bool ClangdLSPServer::run(std::FILE *In, bool TestMode,
+                          JSONStreamStyle InputStyle) {
   assert(!IsDone && "Run was called before");
 
   // Set up JSONRPCDispatcher.
   JSONRPCDispatcher Dispatcher([](const json::Value &Params) {
     replyError(ErrorCode::MethodNotFound, "method not found");
   });
   registerCallbackHandlers(Dispatcher, /*Callbacks=*/*this);
 
   // Run the Language Server loop.
-  runLanguageServerLoop(In, Out, InputStyle, Dispatcher, IsDone);
+  runLanguageServerLoop(In, Out, InputStyle, Dispatcher, IsDone, TestMode);
 
   // Make sure IsDone is set to true after this method exits to ensure assertion
   // at the start of the method fires if it's ever executed again.
   IsDone = true;
 
   return ShutdownRequestReceived;
 }
 

JSONRPCDispatcher.h

 /// After handling each query checks if \p IsDone is set true and exits the loop
 /// if it is.
 /// Input stream(\p In) must be opened in binary mode to avoid preliminary
 /// replacements of \r\n with \n.
 /// We use C-style FILE* for reading as std::istream has unclear interaction
 /// with signals, which are sent by debuggers on some OSs.
 void runLanguageServerLoop(std::FILE *In, JSONOutput &Out,
                            JSONStreamStyle InputStyle,
-                           JSONRPCDispatcher &Dispatcher, bool &IsDone);
+                           JSONRPCDispatcher &Dispatcher, bool &IsDone,
+                           bool TestMode);
 
 } // namespace clangd
 } // namespace clang
 
 #endif

JSONRPCDispatcher.cpp

 // Previously, std::istream was used. When a debugger attached on MacOS, the
 // process received EINTR, the stream went bad, and clangd exited.
 // A retry-on-EINTR loop around reads solved this problem, but caused clangd to
 // sometimes hang rather than exit on other OSes. The interaction between
 // istreams and signals isn't well-specified, so it's hard to get this right.
 // The C APIs seem to be clearer in this respect.
 void clangd::runLanguageServerLoop(std::FILE *In, JSONOutput &Out,
                                    JSONStreamStyle InputStyle,
-                                   JSONRPCDispatcher &Dispatcher,
-                                   bool &IsDone) {
+                                   JSONRPCDispatcher &Dispatcher, bool &IsDone,
+                                   bool TestMode) {
   auto &ReadMessage =
       (InputStyle == Delimited) ? readDelimitedMessage : readStandardMessage;
   while (!IsDone && !feof(In)) {
     if (ferror(In)) {
       elog("IO error: {0}", llvm::sys::StrError());
       return;
     }
     if (auto JSON = ReadMessage(In, Out)) {
       if (auto Doc = json::parse(*JSON)) {
         // Log the formatted message.
         vlog(Out.Pretty ? "<<< {0:2}\n" : "<<< {0}\n", *Doc);
         // Finally, execute the action for this JSON message.
         if (!Dispatcher.call(*Doc, Out))
           elog("JSON dispatch failed!");
       } else {
         // Parse error. Log the raw message.
         vlog("<<< {0}\n", *JSON);
         elog("JSON parse error: {0}", llvm::toString(Doc.takeError()));
+        if (TestMode)
+          exit(EXIT_FAILURE);
       }
     }
   }
 }

tool/ClangdMain.cpp

int main(int argc, char *argv[]) {
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
       Out, CCOpts, CompileCommandsDirPath,
       /*ShouldUseInMemoryCDB=*/CompileArgsFrom == LSPCompileArgs, Opts);
   constexpr int NoShutdownRequestErrorCode = 1;
   llvm::set_thread_name("clangd.main");
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();
-  return LSPServer.run(stdin, InputStyle) ? 0 : NoShutdownRequestErrorCode;
+  return LSPServer.run(stdin, Test, InputStyle) ? 0
+                                                : NoShutdownRequestErrorCode;
 }