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

[Pavel Labath via Phabricator via lldb-commits]

labath marked an inline comment as done.
labath added a comment.





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


================
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) {
----------------
zturner wrote:
> How hard would it be to change `categories` to `ArrayRef<StringRef>`, or alternatively `ArrayRef<const char*>`?
The second one is moderately easy, if we add the appropriate accessor to the Args class. However, I'd leave that for later. If I do that now, I'd have to update at least a dozen functions. After the refactor it should be just two.


================
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);
+  }
----------------
zturner wrote:
> 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.
I was not tried to address these kinds of races here. I am fine with assuming that the calls enabling/disabling log channels are externally serialized (same as the previous implementation).

What I am tried to address is the race between someone enabling a log and someone else attempting to write to the log (e.g. GetLogIfAllCategories set). This is something that should be done with as low overhead as possible (i.e., no mutex), as that code will be running even when logging is disabled. This is why I am using an atomic variable. The original code was using a simple pointer and hoping that the update will be atomic, which is not correct, so I am trying to improve that here.

Note that this is still not completely data-race free as the flags update is not done atomically, but that is also something that I am not trying to solve here. I think I'll try to make another patch later which just deals with the thread-safety, this is just something that I wanted to sneak in as I was already modifying this part.


================
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);
 }
----------------
zturner wrote:
> Where is `g_log_channel_` allocated?  AFAICT it's always `nullptr`.
It is set in the Log.cpp file, when we enable the logging. This was not a very good design, as I have smeared the atomic accesses over two files. The next one version of this patch should compartmentalize it better.


https://reviews.llvm.org/D29895