[Lldb-commits] [PATCH] D30702: Fix remaining threading issues in Log.h

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.





================
Comment at: include/lldb/Utility/Log.h:152
 
-  Flags &GetOptions();
+  const Flags GetOptions() const;
 
----------------
eugene wrote:
> Seems like const on return value is not really needed.
Well... otherwise people could write something like: `log.GetOptions().Reset(foo)` and expect that it will change underlying log object. This worked before this patch, so this seems like a good idea -- make sure accidental usage like that results in a compiler error rather than silently fail.


================
Comment at: include/lldb/Utility/Log.h:163
+  std::atomic<uint32_t> m_options{0};
+  std::atomic<uint32_t> m_mask{0};
+
----------------
eugene wrote:
> Do we actually need atomics here? 
> 
> It seems like the stream itself is protected by the mutex, and It doesn't seem to affect performance.
> Can we use the same (or a different) mutex to protect flags and options?
The difference is that these variables are accessed even when logging is disabled -- `GetLogIfAll` fetches the mask to decide whether it should log things. The stream mutex is only taken if logging is enabled **and** you are actually logging something (also only the read side is taken, so it is contention-free unless someone actually is modifying the stream at the same time).

I don't think doing this with a mutex would drastically hurt our performance, but I'm trying to avoid the case where enabling logging introduces enough thread synchronization than the race condition you were debugging disappears.


================
Comment at: source/Utility/Log.cpp:90-91
                           uint32_t options, uint32_t flags) {
-  log.GetMask().Set(flags);
-  if (log.GetMask().Get()) {
-    log.GetOptions().Reset(options);
+  uint32_t mask = log.m_mask.load(std::memory_order_acquire) | flags;
+  log.m_mask.store(mask, std::memory_order_release);
+  if (mask) {
----------------
zturner wrote:
> How about 
> 
> ```
> uint32_t old_mask = log.m_mask.fetch_or(flags);
> if (old_mask  & flags) {
>   ...
> }
> ```
> 
> Also, there is still a race if two people call this function at the same time.
That is not the race I was trying to solve here, but it seems making sure that is safe wouldn't be too hard, and it should make this easier to reason about. I'll reuse the stream mutex for this case.


================
Comment at: source/Utility/Log.cpp:116
+const Flags Log::GetMask() const {
+  return m_mask.load(std::memory_order_acquire);
+}
----------------
zturner wrote:
> Any reason you always use the explicit `load(memory_order_acquire)` syntax instead of using the default sequentially consistent ordering?
The sequentially consistent ordering is more heavy, particularly on weakly ordered systems, and it's not necessary here - all I need is to make sure the value is read correctly. In fact, I was considering dropping this all the way to memory_order_relaxed.


https://reviews.llvm.org/D30702