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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
