Addressing reviewer feedback and fixing client_log having the incorrect scope when lldb-dap is running in server mode on accept.

Author: ashgti

lldb/tools/lldb-dap/DAP.h

@@ -23,6 +23,7 @@
 #include "Transport.h"
 #include "Variables.h"
 #include "lldb/API/SBBroadcaster.h"
+#include "lldb/API/SBCommandInterpreter.h"
 #include "lldb/API/SBDebugger.h"
 #include "lldb/API/SBError.h"
 #include "lldb/API/SBFile.h"

lldb/tools/lldb-dap/DAPLog.h

@@ -41,7 +41,7 @@ namespace lldb_dap {
 /// `DAP_LOG_ERROR` helpers.
 class Log final {
 public:
-  using Mutex = std::recursive_mutex;
+  using Mutex = std::mutex;
 
   Log(llvm::raw_ostream &stream, Mutex &mutex)
       : m_stream(stream), m_mutex(mutex) {}
@@ -51,7 +51,7 @@ class Log final {
   /// Retuns a new Log instance with the associated prefix for all messages.
   inline Log WithPrefix(llvm::StringRef prefix) const {
     std::string full_prefix =
-        m_prefix.empty() ? prefix.str() : m_prefix + " " + prefix.str();
+        m_prefix.empty() ? prefix.str() : m_prefix + prefix.str();
     full_prefix += " ";
     return Log(full_prefix, *this);
   }

lldb/tools/lldb-dap/EventHelper.cpp

@@ -480,7 +480,7 @@ static void HandleTargetEvent(const lldb::SBEvent &event, Log &log) {
   }
 }
 
-void HandleBreakpointEvent(const lldb::SBEvent &event, Log &log) {
+static void HandleBreakpointEvent(const lldb::SBEvent &event, Log &log) {
   const uint32_t event_mask = event.GetType();
   if (!(event_mask & lldb::SBTarget::eBroadcastBitBreakpointChanged))
     return;

lldb/tools/lldb-dap/Handler/CompletionsHandler.cpp

@@ -7,11 +7,11 @@
 //===----------------------------------------------------------------------===//
 
 #include "DAP.h"
+#include "JSONUtils.h"
 #include "Protocol/ProtocolRequests.h"
 #include "Protocol/ProtocolTypes.h"
 #include "RequestHandler.h"
 #include "lldb/API/SBStringList.h"
-#include "lldb/SBCommandInterpreter.h"
 
 using namespace llvm;
 using namespace lldb_dap;

lldb/tools/lldb-dap/tool/lldb-dap.cpp

@@ -453,15 +453,16 @@ static llvm::Error serveConnection(
       ResetConnectionTimeout(g_connection_timeout_mutex,
                              g_connection_timeout_time_point);
     std::string client_name = llvm::formatv("client_{0}", clientCount++).str();
-    Log client_log = log.WithPrefix("(" + client_name + ")");
-    DAP_LOG(client_log, "client connected");
-
     lldb::IOObjectSP io(std::move(sock));
 
     // Move the client into a background thread to unblock accepting the next
     // client.
-    std::thread client([=, &client_log]() {
+    std::thread client([=, &log]() {
       llvm::set_thread_name(client_name + ".runloop");
+
+      Log client_log = log.WithPrefix("(" + client_name + ")");
+      DAP_LOG(client_log, "client connected");
+
       MainLoop loop;
       Transport transport(client_log, io, io);
       DAP dap(client_log, default_repl_mode, pre_init_commands, no_lldbinit,
@@ -652,7 +653,7 @@ int main(int argc, char *argv[]) {
     log_os = std::make_unique<llvm::raw_fd_ostream>(FD, /*shouldClose=*/true);
   }
   Log::Mutex mutex;
-  Log log(log_os ? *log_os.get() : llvm::nulls(), mutex);
+  Log log(log_os ? *log_os : llvm::nulls(), mutex);
 
   // Initialize LLDB first before we do anything.
   lldb::SBError error = lldb::SBDebugger::InitializeWithErrorHandling();

lldb/unittests/DAP/DAPLogTest.cpp

@@ -8,10 +8,12 @@
 
 #include "DAPLog.h"
 #include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using namespace lldb_dap;
 using namespace llvm;
+using namespace testing;
 
 static llvm::StringRef last_line(llvm::StringRef str) {
   size_t index = str.find_last_of('\n', str.size() - 1);
@@ -20,27 +22,32 @@ static llvm::StringRef last_line(llvm::StringRef str) {
   return str.substr(index + 1);
 }
 
+#define TIMESTAMP_PATTERN "[0-9]+\\.[0-9]+ "
+
 TEST(DAPLog, Emit) {
   Log::Mutex mux;
   std::string outs;
   raw_string_ostream os(outs);
   Log log(os, mux);
   Log inner_log = log.WithPrefix("my_prefix:");
 
-  // Line includes a timestamp, only check the suffix.
   log.Emit("Hi");
-  EXPECT_TRUE(last_line(outs).ends_with(" Hi\n")) << outs;
+  EXPECT_THAT(last_line(outs), MatchesRegex(TIMESTAMP_PATTERN "Hi\n"));
 
   inner_log.Emit("foobar");
-  EXPECT_TRUE(last_line(outs).ends_with(" my_prefix: foobar\n")) << outs;
+  EXPECT_THAT(last_line(outs),
+              MatchesRegex(TIMESTAMP_PATTERN "my_prefix: foobar\n"));
 
   log.Emit("file.cpp", 42, "Hello from a file/line.");
-  EXPECT_TRUE(
-      last_line(outs).ends_with(" file.cpp:42 Hello from a file/line.\n"))
-      << outs;
+  EXPECT_THAT(
+      last_line(outs),
+      MatchesRegex(TIMESTAMP_PATTERN "file.cpp:42 Hello from a file/line.\n"));
 
   inner_log.Emit("file.cpp", 42, "Hello from a file/line.");
-  EXPECT_TRUE(last_line(outs).ends_with(
-      " file.cpp:42 my_prefix: Hello from a file/line.\n"))
-      << outs;
-}
+  EXPECT_THAT(last_line(outs),
+              MatchesRegex(TIMESTAMP_PATTERN
+                           "file.cpp:42 my_prefix: Hello from a file/line.\n"));
+
+  log.WithPrefix("a").WithPrefix("b").WithPrefix("c").Emit("msg");
+  EXPECT_THAT(last_line(outs), MatchesRegex(TIMESTAMP_PATTERN "a b c msg\n"));
+}

lldb/unittests/DAP/TestBase.cpp

@@ -39,7 +39,7 @@ void TransportBase::SetUp() {
 
   log = std::make_unique<Log>(llvm::outs(), log_mutex);
   dap = std::make_unique<DAP>(
-      /*log=*/*log.get(),
+      /*log=*/*log,
       /*default_repl_mode=*/ReplMode::Auto,
       /*pre_init_commands=*/std::vector<std::string>(),
       /*no_lldbinit=*/false,