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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline 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)
----------------
clayborg wrote:
> Why wouldn't we just add all of the categories in the enum? Are we trying to reduce code changes?
Yes, the full patch would definitely do that. This was just something I quickly whipped up to demonstrate how I thought this could work.


================
Comment at: lldb/include/lldb/Utility/Logging.h:20
+  Process = 1u << 1,
+  LLVM_MARK_AS_BITMASK_ENUM(UINT32_MAX)
+};
----------------
clayborg wrote:
> 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);
> ```
The idea is that there will be a single (templated) `GetLogIfAny/All` function (see Log.h:218), and that one will automatically select (based on the type of the enum) the right channel.


================
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)
----------------
clayborg wrote:
> Seems like using LLDBLog::Process everywhere instead of LIBLLDB_LOG_PROCESS would be cleaner? Are we just trying to reduce code changes?
Yes, the goal would be do delete these completely, but I'd split this into two patch -- first do the functional changes, and then do a search-replace to get rid of all the macros.


================
Comment at: lldb/include/lldb/Utility/Logging.h:64-66
+Log *GetLogIfAllCategoriesSet(LLDBLog mask);
 
+Log *GetLogIfAnyCategoriesSet(LLDBLog mask);
----------------
These functions are also just temporary


================
Comment at: lldb/include/lldb/Utility/Logging.h:70
 
+template <> Log::Channel &LogChannelFor<LLDBLog>();
 } // namespace lldb_private
----------------
This is what ties an enum to a particular channel.


================
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)},
----------------
clayborg wrote:
> Should the Log::Category be templatized with GDBRLog and then we just store GDBRLog enum values instead of uint32_t values?
The problem with that is that it would require templatizing pretty much all of the logging code (as it deals with categories in one way or another), but I agree that we should at least make the category definitions nice. It should be possible to keep templates here, but then convert to integers when we get to the core logging code, but i did not want to get entagled in that for the prototype.


================
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,
----------------
clayborg wrote:
> I would love these to be cleaner. Maybe something like:
> ```
> Log *log = GDBRLog.GetLogIfAny(GDBRLog::Thread);
> ```
`Log *log = GetLogIfAny(GDBRLog::Thread);` is the final goal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117490