llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: John Harrison (ashgti) <details> <summary>Changes</summary> Today, logging is handling by a simple `std::ofstream *` for handling logging. LLDBLog.h can help improve logging by adding new categories of logs and give us additional formatting support for log messages. We link against the libHost, which includes lldbUtility. This also simplifies logging by not requiring the `std::ofstream *` pointer being passed to every function that includes logging. --- Full diff: https://github.com/llvm/llvm-project/pull/129294.diff 9 Files Affected: - (modified) lldb/tools/lldb-dap/CMakeLists.txt (+1) - (modified) lldb/tools/lldb-dap/DAP.cpp (+20-42) - (modified) lldb/tools/lldb-dap/DAP.h (+5-6) - (added) lldb/tools/lldb-dap/DAPLog.cpp (+22) - (added) lldb/tools/lldb-dap/DAPLog.h (+34) - (modified) lldb/tools/lldb-dap/EventHelper.cpp (+10-7) - (modified) lldb/tools/lldb-dap/IOStream.cpp (+9-10) - (modified) lldb/tools/lldb-dap/IOStream.h (+3-4) - (modified) lldb/tools/lldb-dap/lldb-dap.cpp (+42-33) ``````````diff diff --git a/lldb/tools/lldb-dap/CMakeLists.txt b/lldb/tools/lldb-dap/CMakeLists.txt index 8b3c520ec4360..d9f09f6d022ed 100644 --- a/lldb/tools/lldb-dap/CMakeLists.txt +++ b/lldb/tools/lldb-dap/CMakeLists.txt @@ -23,6 +23,7 @@ add_lldb_tool(lldb-dap Breakpoint.cpp BreakpointBase.cpp DAP.cpp + DAPLog.cpp EventHelper.cpp ExceptionBreakpoint.cpp FifoFiles.cpp diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp index 53c514b790f38..81f5205d4f6bd 100644 --- a/lldb/tools/lldb-dap/DAP.cpp +++ b/lldb/tools/lldb-dap/DAP.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "DAP.h" +#include "DAPLog.h" #include "Handler/ResponseHandler.h" #include "JSONUtils.h" #include "LLDBUtils.h" @@ -19,6 +20,7 @@ #include "lldb/API/SBProcess.h" #include "lldb/API/SBStream.h" #include "lldb/Utility/IOObject.h" +#include "lldb/Utility/Log.h" #include "lldb/Utility/Status.h" #include "lldb/lldb-defines.h" #include "lldb/lldb-enumerations.h" @@ -50,6 +52,7 @@ #endif using namespace lldb_dap; +using namespace lldb_private; namespace { #ifdef _WIN32 @@ -61,13 +64,12 @@ const char DEV_NULL[] = "/dev/null"; namespace lldb_dap { -DAP::DAP(std::string name, llvm::StringRef path, std::ofstream *log, - lldb::IOObjectSP input, lldb::IOObjectSP output, ReplMode repl_mode, +DAP::DAP(std::string name, llvm::StringRef path, lldb::IOObjectSP input, + lldb::IOObjectSP output, ReplMode repl_mode, std::vector<std::string> pre_init_commands) - : name(std::move(name)), debug_adapter_path(path), log(log), - input(std::move(input)), output(std::move(output)), - broadcaster("lldb-dap"), exception_breakpoints(), - pre_init_commands(std::move(pre_init_commands)), + : name(std::move(name)), debug_adapter_path(path), input(std::move(input)), + output(std::move(output)), broadcaster("lldb-dap"), + exception_breakpoints(), pre_init_commands(std::move(pre_init_commands)), focus_tid(LLDB_INVALID_THREAD_ID), stop_at_entry(false), is_attach(false), enable_auto_variable_summaries(false), enable_synthetic_child_debugging(false), @@ -245,6 +247,8 @@ void DAP::SendJSON(const std::string &json_str) { output.write_full(llvm::utostr(json_str.size())); output.write_full("\r\n\r\n"); output.write_full(json_str); + + LLDB_LOG(GetLog(DAPLog::Transport), "{0} <-- {1}", name, json_str); } // Serialize the JSON value into a string and send the JSON packet to @@ -256,15 +260,6 @@ void DAP::SendJSON(const llvm::json::Value &json) { static std::mutex mutex; std::lock_guard<std::mutex> locker(mutex); SendJSON(json_str); - - if (log) { - auto now = std::chrono::duration<double>( - std::chrono::system_clock::now().time_since_epoch()); - *log << llvm::formatv("{0:f9} {1} <-- ", now.count(), name).str() - << std::endl - << "Content-Length: " << json_str.size() << "\r\n\r\n" - << llvm::formatv("{0:2}", json).str() << std::endl; - } } // Read a JSON packet from the "in" stream. @@ -273,28 +268,22 @@ std::string DAP::ReadJSON() { std::string json_str; int length; - if (!input.read_expected(log, "Content-Length: ")) + if (!input.read_expected("Content-Length: ")) return json_str; - if (!input.read_line(log, length_str)) + if (!input.read_line(length_str)) return json_str; if (!llvm::to_integer(length_str, length)) return json_str; - if (!input.read_expected(log, "\r\n")) + if (!input.read_expected("\r\n")) return json_str; - if (!input.read_full(log, length, json_str)) + if (!input.read_full(length, json_str)) return json_str; - if (log) { - auto now = std::chrono::duration<double>( - std::chrono::system_clock::now().time_since_epoch()); - *log << llvm::formatv("{0:f9} {1} --> ", now.count(), name).str() - << std::endl - << "Content-Length: " << length << "\r\n\r\n"; - } + LLDB_LOG(GetLog(DAPLog::Transport), "{0} --> {1}", name, json_str); return json_str; } @@ -729,24 +718,14 @@ PacketStatus DAP::GetNextObject(llvm::json::Object &object) { llvm::Expected<llvm::json::Value> json_value = llvm::json::parse(json_sref); if (!json_value) { auto error = json_value.takeError(); - if (log) { - std::string error_str; - llvm::raw_string_ostream strm(error_str); - strm << error; - *log << "error: failed to parse JSON: " << error_str << std::endl - << json << std::endl; - } + LLDB_LOG_ERROR(GetLog(DAPLog::Protocol), std::move(error), + "failed to parse JSON: {0}"); return PacketStatus::JSONMalformed; } - if (log) { - *log << llvm::formatv("{0:2}", *json_value).str() << std::endl; - } - llvm::json::Object *object_ptr = json_value->getAsObject(); if (!object_ptr) { - if (log) - *log << "error: json packet isn't a object" << std::endl; + LLDB_LOG(GetLog(DAPLog::Protocol), "error: json packet isn't a object"); return PacketStatus::JSONNotObject; } object = *object_ptr; @@ -764,9 +743,8 @@ bool DAP::HandleObject(const llvm::json::Object &object) { return true; // Success } - if (log) - *log << "error: unhandled command \"" << command.data() << "\"" - << std::endl; + LLDB_LOG(GetLog(DAPLog::Protocol), "error: unhandled command '{0}'", + command); return false; // Fail } diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h index 8b2e498a28c95..ea5e17eb5d95f 100644 --- a/lldb/tools/lldb-dap/DAP.h +++ b/lldb/tools/lldb-dap/DAP.h @@ -125,21 +125,21 @@ struct Variables { struct StartDebuggingRequestHandler : public lldb::SBCommandPluginInterface { DAP &dap; - explicit StartDebuggingRequestHandler(DAP &d) : dap(d) {}; + explicit StartDebuggingRequestHandler(DAP &d) : dap(d){}; bool DoExecute(lldb::SBDebugger debugger, char **command, lldb::SBCommandReturnObject &result) override; }; struct ReplModeRequestHandler : public lldb::SBCommandPluginInterface { DAP &dap; - explicit ReplModeRequestHandler(DAP &d) : dap(d) {}; + explicit ReplModeRequestHandler(DAP &d) : dap(d){}; bool DoExecute(lldb::SBDebugger debugger, char **command, lldb::SBCommandReturnObject &result) override; }; struct SendEventRequestHandler : public lldb::SBCommandPluginInterface { DAP &dap; - explicit SendEventRequestHandler(DAP &d) : dap(d) {}; + explicit SendEventRequestHandler(DAP &d) : dap(d){}; bool DoExecute(lldb::SBDebugger debugger, char **command, lldb::SBCommandReturnObject &result) override; }; @@ -147,7 +147,6 @@ struct SendEventRequestHandler : public lldb::SBCommandPluginInterface { struct DAP { std::string name; llvm::StringRef debug_adapter_path; - std::ofstream *log; InputStream input; OutputStream output; lldb::SBFile in; @@ -210,8 +209,8 @@ struct DAP { // will contain that expression. std::string last_nonempty_var_expression; - DAP(std::string name, llvm::StringRef path, std::ofstream *log, - lldb::IOObjectSP input, lldb::IOObjectSP output, ReplMode repl_mode, + DAP(std::string name, llvm::StringRef path, lldb::IOObjectSP input, + lldb::IOObjectSP output, ReplMode repl_mode, std::vector<std::string> pre_init_commands); ~DAP(); DAP(const DAP &rhs) = delete; diff --git a/lldb/tools/lldb-dap/DAPLog.cpp b/lldb/tools/lldb-dap/DAPLog.cpp new file mode 100644 index 0000000000000..840e995b14af8 --- /dev/null +++ b/lldb/tools/lldb-dap/DAPLog.cpp @@ -0,0 +1,22 @@ +#include "DAPLog.h" + +using namespace lldb_private; +using namespace lldb_dap; + +static constexpr Log::Category g_categories[] = { + {{"transport"}, {"log DAP transport"}, DAPLog::Transport}, + {{"protocol"}, {"log protocol handling"}, DAPLog::Protocol}, + {{"connection"}, {"log connection handling"}, DAPLog::Connection}, +}; + +static Log::Channel g_log_channel(g_categories, DAPLog::Transport | + DAPLog::Protocol | + DAPLog::Connection); + +template <> Log::Channel &lldb_private::LogChannelFor<DAPLog>() { + return g_log_channel; +} + +void lldb_dap::InitializeDAPChannel() { + Log::Register("lldb-dap", g_log_channel); +} diff --git a/lldb/tools/lldb-dap/DAPLog.h b/lldb/tools/lldb-dap/DAPLog.h new file mode 100644 index 0000000000000..49be7602b2c88 --- /dev/null +++ b/lldb/tools/lldb-dap/DAPLog.h @@ -0,0 +1,34 @@ +//===-- DAPLog.h ------------------------------------------------*- C++ -*-===// +// +// 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 +// +//===----------------------------------------------------------------------===// + +#ifndef LLDB_UTILITY_LLDBLOG_H +#define LLDB_UTILITY_LLDBLOG_H + +#include "lldb/Utility/Log.h" +#include "llvm/ADT/BitmaskEnum.h" + +namespace lldb_dap { + +enum class DAPLog : lldb_private::Log::MaskType { + Transport = lldb_private::Log::ChannelFlag<0>, + Protocol = lldb_private::Log::ChannelFlag<1>, + Connection = lldb_private::Log::ChannelFlag<2>, + LLVM_MARK_AS_BITMASK_ENUM(Connection), +}; + +LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE(); + +void InitializeDAPChannel(); + +} // end namespace lldb_dap + +namespace lldb_private { +template <> lldb_private::Log::Channel &LogChannelFor<lldb_dap::DAPLog>(); +} // namespace lldb_private + +#endif diff --git a/lldb/tools/lldb-dap/EventHelper.cpp b/lldb/tools/lldb-dap/EventHelper.cpp index 705eb0a457d9c..299d8f8a482ac 100644 --- a/lldb/tools/lldb-dap/EventHelper.cpp +++ b/lldb/tools/lldb-dap/EventHelper.cpp @@ -8,6 +8,7 @@ #include "EventHelper.h" #include "DAP.h" +#include "DAPLog.h" #include "JSONUtils.h" #include "LLDBUtils.h" #include "lldb/API/SBFileSpec.h" @@ -21,6 +22,9 @@ #endif #endif +using namespace lldb_dap; +using namespace lldb_private; + namespace lldb_dap { static void SendThreadExitedEvent(DAP &dap, lldb::tid_t tid) { @@ -178,15 +182,14 @@ void SendThreadStoppedEvent(DAP &dap) { SendThreadExitedEvent(dap, tid); } } else { - if (dap.log) - *dap.log << "error: SendThreadStoppedEvent() when process" - " isn't stopped (" - << lldb::SBDebugger::StateAsCString(state) << ')' << std::endl; + LLDB_LOG(GetLog(DAPLog::Protocol), + "error: SendThreadStoppedEvent() when process" + " isn't stopped ({0})", + lldb::SBDebugger::StateAsCString(state)); } } else { - if (dap.log) - *dap.log << "error: SendThreadStoppedEvent() invalid process" - << std::endl; + LLDB_LOG(GetLog(DAPLog::Protocol), + "error: SendThreadStoppedEvent() invalid process"); } dap.RunStopCommands(); } diff --git a/lldb/tools/lldb-dap/IOStream.cpp b/lldb/tools/lldb-dap/IOStream.cpp index c6f1bfaf3b799..0e93ce8111891 100644 --- a/lldb/tools/lldb-dap/IOStream.cpp +++ b/lldb/tools/lldb-dap/IOStream.cpp @@ -7,12 +7,13 @@ //===----------------------------------------------------------------------===// #include "IOStream.h" +#include "DAPLog.h" #include "lldb/Utility/IOObject.h" #include "lldb/Utility/Status.h" -#include <fstream> #include <string> using namespace lldb_dap; +using namespace lldb_private; bool OutputStream::write_full(llvm::StringRef str) { if (!descriptor) @@ -23,8 +24,7 @@ bool OutputStream::write_full(llvm::StringRef str) { return status.Success(); } -bool InputStream::read_full(std::ofstream *log, size_t length, - std::string &text) { +bool InputStream::read_full(size_t length, std::string &text) { if (!descriptor) return false; @@ -39,10 +39,10 @@ bool InputStream::read_full(std::ofstream *log, size_t length, return true; } -bool InputStream::read_line(std::ofstream *log, std::string &line) { +bool InputStream::read_line(std::string &line) { line.clear(); while (true) { - if (!read_full(log, 1, line)) + if (!read_full(1, line)) return false; if (llvm::StringRef(line).ends_with("\r\n")) @@ -52,14 +52,13 @@ bool InputStream::read_line(std::ofstream *log, std::string &line) { return true; } -bool InputStream::read_expected(std::ofstream *log, llvm::StringRef expected) { +bool InputStream::read_expected(llvm::StringRef expected) { std::string result; - if (!read_full(log, expected.size(), result)) + if (!read_full(expected.size(), result)) return false; if (expected != result) { - if (log) - *log << "Warning: Expected '" << expected.str() << "', got '" << result - << "\n"; + LLDB_LOG(GetLog(DAPLog::Transport), "Warning: Expected '{0}', got '{1}'", + expected, result); } return true; } diff --git a/lldb/tools/lldb-dap/IOStream.h b/lldb/tools/lldb-dap/IOStream.h index e9fb8e11c92da..c90fee57d0479 100644 --- a/lldb/tools/lldb-dap/IOStream.h +++ b/lldb/tools/lldb-dap/IOStream.h @@ -11,7 +11,6 @@ #include "lldb/lldb-forward.h" #include "llvm/ADT/StringRef.h" -#include <fstream> #include <string> namespace lldb_dap { @@ -22,11 +21,11 @@ struct InputStream { explicit InputStream(lldb::IOObjectSP descriptor) : descriptor(std::move(descriptor)) {} - bool read_full(std::ofstream *log, size_t length, std::string &text); + bool read_full(size_t length, std::string &text); - bool read_line(std::ofstream *log, std::string &line); + bool read_line(std::string &line); - bool read_expected(std::ofstream *log, llvm::StringRef expected); + bool read_expected(llvm::StringRef expected); }; struct OutputStream { diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp index d005eccfae903..2c337edc86b89 100644 --- a/lldb/tools/lldb-dap/lldb-dap.cpp +++ b/lldb/tools/lldb-dap/lldb-dap.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "DAP.h" +#include "DAPLog.h" #include "EventHelper.h" #include "Handler/RequestHandler.h" #include "RunInTerminal.h" @@ -71,13 +72,7 @@ typedef int socklen_t; #endif using namespace lldb_dap; -using lldb_private::File; -using lldb_private::IOObject; -using lldb_private::MainLoop; -using lldb_private::MainLoopBase; -using lldb_private::NativeFile; -using lldb_private::Socket; -using lldb_private::Status; +using namespace lldb_private; namespace { using namespace llvm::opt; @@ -277,8 +272,7 @@ validateConnection(llvm::StringRef conn) { static llvm::Error serveConnection(const Socket::SocketProtocol &protocol, const std::string &name, - std::ofstream *log, llvm::StringRef program_path, - const ReplMode default_repl_mode, + llvm::StringRef program_path, const ReplMode default_repl_mode, const std::vector<std::string> &pre_init_commands) { Status status; static std::unique_ptr<Socket> listener = Socket::Create(protocol, status); @@ -292,8 +286,8 @@ serveConnection(const Socket::SocketProtocol &protocol, const std::string &name, } std::string address = llvm::join(listener->GetListeningConnectionURI(), ", "); - if (log) - *log << "started with connection listeners " << address << "\n"; + LLDB_LOG(GetLog(DAPLog::Connection), "started with connection listeners {0}", + address); llvm::outs() << "Listening for: " << address << "\n"; // Ensure listening address are flushed for calles to retrieve the resolve @@ -314,12 +308,7 @@ serveConnection(const Socket::SocketProtocol &protocol, const std::string &name, &clientCount]( std::unique_ptr<Socket> sock) { std::string name = llvm::formatv("client_{0}", clientCount++).str(); - if (log) { - auto now = std::chrono::duration<double>( - std::chrono::system_clock::now().time_since_epoch()); - *log << llvm::formatv("{0:f9}", now.count()).str() - << " client connected: " << name << "\n"; - } + LLDB_LOG(GetLog(DAPLog::Connection), "client ({0}) connected", name); lldb::IOObjectSP io(std::move(sock)); @@ -328,8 +317,8 @@ serveConnection(const Socket::SocketProtocol &protocol, const std::string &name, std::thread client([=, &dap_sessions_condition, &dap_sessions_mutex, &dap_sessions]() { llvm::set_thread_name(name + ".runloop"); - DAP dap = DAP(name, program_path, log, io, io, default_repl_mode, - pre_init_commands); + DAP dap = + DAP(name, program_path, io, io, default_repl_mode, pre_init_commands); if (auto Err = dap.ConfigureIO()) { llvm::logAllUnhandledErrors(std::move(Err), llvm::errs(), @@ -349,12 +338,7 @@ serveConnection(const Socket::SocketProtocol &protocol, const std::string &name, "DAP session error: "); } - if (log) { - auto now = std::chrono::duration<double>( - std::chrono::system_clock::now().time_since_epoch()); - *log << llvm::formatv("{0:f9}", now.count()).str() - << " client closed: " << name << "\n"; - } + LLDB_LOG(GetLog(DAPLog::Connection), "client ({0}) closed", name); std::unique_lock<std::mutex> lock(dap_sessions_mutex); dap_sessions.erase(io.get()); @@ -372,9 +356,8 @@ serveConnection(const Socket::SocketProtocol &protocol, const std::string &name, return status.takeError(); } - if (log) - *log << "lldb-dap server shutdown requested, disconnecting remaining " - "clients...\n"; + LLDB_LOG(GetLog(DAPLog::Connection), + "shutdown requested, disconnecting remaining clients"); bool client_failed = false; { @@ -402,6 +385,22 @@ serveConnection(const Socket::SocketProtocol &protocol, const std::string &name, return llvm::Error::success(); } +class DAPLogHandler : public lldb_private::LogHandler { +public: + DAPLogHandler(std::unique_ptr<std::ofstream> stream) + : m_stream(std::move(stream)) {} + + void Emit(llvm::StringRef message) override { + std::lock_guard<std::mutex> guard(m_mutex); + (*m_stream) << message.str(); + m_stream->flush(); + } + +private: + std::mutex m_mutex; + std::unique_ptr<std::ofstream> m_stream; +}; + int main(int argc, char *argv[]) { llvm::InitLLVM IL(argc, argv, /*InstallPipeSignalExitHandler=*/false); #if !defined(__APPLE__) @@ -490,10 +489,20 @@ int main(int argc, char *argv[]) { } #endif - std::unique_ptr<std::ofstream> log = nullptr; + InitializeDAPChannel(); + const char *log_file_path = getenv("LLDBDAP_LOG"); - if (log_file_path) - log = std::make_unique<std::ofstream>(log_file_path); + if (log_file_path) { + std::unique_ptr<std::ofstream> log = + std::make_unique<std::ofstream>(log_file_path); + + if (!lldb_private::Log::EnableLogChannel( + std::make_shared<DAPLogHandler>(std::move(log)), + LLDB_LOG_OPTION_PREPEND_TIMESTAMP, "lldb-dap", {"all"}, + llvm::errs())) { + return EXIT_FAILURE; + } + } // Initialize LLDB first before we do anything. lldb::SBError error = lldb::SBDebugger::InitializeWithErrorHandling(); @@ -525,7 +534,7 @@ int main(int argc, char *argv[]) { Socket::SocketProtocol protocol; std::string name; std::tie(protocol, name) = *maybeProtoclAndName; - if (auto Err = serveConnection(protocol, name, log.get(), program_path, + if (auto Err = serveConnection(protocol, name, program_path, default_repl_mode, pre_init_commands)) { llvm::logAllUnhandledErrors(std::move(Err), llvm::errs(), "Connection failed: "); @@ -559,7 +568,7 @@ int main(int argc, char *argv[]) { lldb::IOObjectSP output = std::make_shared<NativeFile>( stdout_fd, File::eOpenOptionWriteOnly, false); - DAP dap = DAP("stdin/stdout", program_path, log.get(), std::move(input), + DAP dap = DAP("stdin/stdout", program_path, std::move(input), std::move(output), default_repl_mode, pre_init_commands); // stdout/stderr redirection to the IDE's console `````````` </details> https://github.com/llvm/llvm-project/pull/129294 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
