[Lldb-commits] [PATCH] D29895: Refactor log channel registration mechanism

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: include/lldb/Core/Log.h:74
+                       llvm::ArrayRef<Category> categories,
+                       uint32_t default_flags, std::atomic<Log *> &log_ptr);
+  static void Unregister(llvm::StringRef name);
----------------
Not sure I like the idea of this being a `std::atomic`.  Why is this necessary?


================
Comment at: source/Commands/CommandObjectLog.cpp:278
+      bool success = true;
+      for (auto &entry : args.entries())
+        success = success && Log::ListChannelCategories(
----------------
`const auto&`?


================
Comment at: source/Core/Log.cpp:57
+
+void ListCategories(Stream &stream, const ChannelMap::value_type &channel) {
+  stream.Format("Logging categories for '{0}':\n", channel.first());
----------------
LLVM guidelines say only classes, structures, and types can be in an anonymous namespace, but not functions.  So this should go out of the anonymous namespace, but be declared as global statics.

Same thing withsubsequent functions.


================
Comment at: source/Core/Log.cpp:72
+    if (llvm::StringRef("all").equals_lower(categories[i])) {
+      flags |= ~0;
+      continue;
----------------
This is a little clearer if you just say `flags = 0xFFFFFFFF` IMO.


================
Comment at: source/Core/Log.cpp:266
     const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
-    uint32_t log_options, const char *channel, const char **categories,
+    uint32_t log_options, llvm::StringRef channel, const char **categories,
     Stream &error_stream) {
----------------
How hard would it be to change `categories` to `ArrayRef<StringRef>`, or alternatively `ArrayRef<const char*>`?


================
Comment at: source/Core/Log.cpp:284-285
+  if (ch->second.log.m_mask_bits.Get()) {
+    ch->second.log.SetStream(log_stream_sp);
+    ch->second.log_ptr.store(&ch->second.log, std::memory_order_release);
+  }
----------------
Here's an example of why I think we need a mutex instead of `std::atomic`.  There seems to be a race condition here if two threads call `EnableLogChannel` and `DisableLogChannel` at the same time.    You can get into a situation where the stream is null but the log is enabled.


================
Comment at: source/Plugins/SymbolFile/DWARF/LogChannelDWARF.cpp:32
 void LogChannelDWARF::Initialize() {
-  PluginManager::RegisterPlugin(GetPluginNameStatic(),
-                                GetPluginDescriptionStatic(),
-                                LogChannelDWARF::CreateInstance);
-}
-
-void LogChannelDWARF::Terminate() {
-  PluginManager::UnregisterPlugin(LogChannelDWARF::CreateInstance);
-}
-
-LogChannel *LogChannelDWARF::CreateInstance() { return new LogChannelDWARF(); }
-
-lldb_private::ConstString LogChannelDWARF::GetPluginNameStatic() {
-  return SymbolFileDWARF::GetPluginNameStatic();
+  Log::Register("dwarf", g_categories, DWARF_LOG_DEFAULT, g_log_channel);
 }
----------------
Where is `g_log_channel_` allocated?  AFAICT it's always `nullptr`.


https://reviews.llvm.org/D29895