[Lldb-commits] [PATCH] D117490: [lldb] Log prototype

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

I like the idea of the templates that would not allow bitfields from one log channel to be used on another log channel as there we a few incorrect ones in the GDB remote process plug-in. See inlined comments



================
Comment at: lldb/include/lldb/Utility/Logging.h:19
+enum class LLDBLog : uint32_t {
+  Process = 1u << 1,
+  LLVM_MARK_AS_BITMASK_ENUM(UINT32_MAX)
----------------
Why wouldn't we just add all of the categories in the enum? Are we trying to reduce code changes?


================
Comment at: lldb/include/lldb/Utility/Logging.h:20
+  Process = 1u << 1,
+  LLVM_MARK_AS_BITMASK_ENUM(UINT32_MAX)
+};
----------------
Can we add members here to grab the log channels directly from the LLLDBLog enum? Something like:

```
static Log *GetLogIfAll(LLDBLog mask);
static Log *GetLogIfAny(LLDBLog mask);
```


================
Comment at: lldb/include/lldb/Utility/Logging.h:26
 // Log Bits specific to logging in lldb
-#define LIBLLDB_LOG_PROCESS (1u << 1)
-#define LIBLLDB_LOG_THREAD (1u << 2)
-#define LIBLLDB_LOG_DYNAMIC_LOADER (1u << 3)
-#define LIBLLDB_LOG_EVENTS (1u << 4)
-#define LIBLLDB_LOG_BREAKPOINTS (1u << 5)
-#define LIBLLDB_LOG_WATCHPOINTS (1u << 6)
-#define LIBLLDB_LOG_STEP (1u << 7)
-#define LIBLLDB_LOG_EXPRESSIONS (1u << 8)
-#define LIBLLDB_LOG_TEMPORARY (1u << 9)
-#define LIBLLDB_LOG_STATE (1u << 10)
-#define LIBLLDB_LOG_OBJECT (1u << 11)
-#define LIBLLDB_LOG_COMMUNICATION (1u << 12)
-#define LIBLLDB_LOG_CONNECTION (1u << 13)
-#define LIBLLDB_LOG_HOST (1u << 14)
-#define LIBLLDB_LOG_UNWIND (1u << 15)
-#define LIBLLDB_LOG_API (1u << 16)
-#define LIBLLDB_LOG_SCRIPT (1u << 17)
-#define LIBLLDB_LOG_COMMANDS (1U << 18)
-#define LIBLLDB_LOG_TYPES (1u << 19)
-#define LIBLLDB_LOG_SYMBOLS (1u << 20)
-#define LIBLLDB_LOG_MODULES (1u << 21)
-#define LIBLLDB_LOG_TARGET (1u << 22)
-#define LIBLLDB_LOG_MMAP (1u << 23)
-#define LIBLLDB_LOG_OS (1u << 24)
-#define LIBLLDB_LOG_PLATFORM (1u << 25)
-#define LIBLLDB_LOG_SYSTEM_RUNTIME (1u << 26)
-#define LIBLLDB_LOG_JIT_LOADER (1u << 27)
-#define LIBLLDB_LOG_LANGUAGE (1u << 28)
-#define LIBLLDB_LOG_DATAFORMATTERS (1u << 29)
-#define LIBLLDB_LOG_DEMANGLE (1u << 30)
-#define LIBLLDB_LOG_AST (1u << 31)
-#define LIBLLDB_LOG_ALL (UINT32_MAX)
+#define LIBLLDB_LOG_PROCESS ::lldb_private::LLDBLog::Process
+#define LIBLLDB_LOG_THREAD ::lldb_private::LLDBLog(1u << 2)
----------------
Seems like using LLDBLog::Process everywhere instead of LIBLLDB_LOG_PROCESS would be cleaner? Are we just trying to reduce code changes?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemoteLog.cpp:17
 
 static constexpr Log::Category g_categories[] = {
+    {{"async"}, {"log asynchronous activity"}, uint32_t(GDBR_LOG_ASYNC)},
----------------
Should the Log::Category be templatized with GDBRLog and then we just store GDBRLog enum values instead of uint32_t values?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemoteLog.cpp:18
 static constexpr Log::Category g_categories[] = {
-    {{"async"}, {"log asynchronous activity"}, GDBR_LOG_ASYNC},
-    {{"break"}, {"log breakpoints"}, GDBR_LOG_BREAKPOINTS},
-    {{"comm"}, {"log communication activity"}, GDBR_LOG_COMM},
-    {{"packets"}, {"log gdb remote packets"}, GDBR_LOG_PACKETS},
-    {{"memory"}, {"log memory reads and writes"}, GDBR_LOG_MEMORY},
+    {{"async"}, {"log asynchronous activity"}, uint32_t(GDBR_LOG_ASYNC)},
+    {{"break"}, {"log breakpoints"}, uint32_t(GDBR_LOG_BREAKPOINTS)},
----------------
If we do templatize Log::Category with GDBRLog (like "Log::Category<GDBRLog>") then we can just use the real enums here?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.cpp:57
   ProcessSP process_sp(GetProcess());
-  Log *log(GetLogIfAnyCategoriesSet(GDBR_LOG_THREAD));
+  Log *log(GetLogIfAny(GDBR_LOG_THREAD));
   LLDB_LOG(log, "this = {0}, pid = {1}, tid = {2}", this,
----------------
I would love these to be cleaner. Maybe something like:
```
Log *log = GDBRLog.GetLogIfAny(GDBRLog::Thread);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117490