[Lldb-commits] [PATCH] D127922: [lldb] Introduce the concept of a log handler (NFC)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D127922#3595530 <https://reviews.llvm.org/D127922#3595530>, @labath wrote:

> I think we should talk about the threadsafe flag. The reason that the non-threadsafe mode is safe now is because we're using unbuffered streams, where each stream operation will map to a single write syscall (which will then be serialized by the OS somehow). And since we pre-format the log messages into a temporary buffer, we shouldn't even get overlapping messages in the output. In fact, I would say that, before this, the threadsafe mode was mostly useless.
>
> All of that goes out the window once you start doing arbitrary logic in place of log writing. As they're implemented now, neither BufferedLogHandler nor RotatingLogHandler are thread-safe. (Making the rotating handler thread safe (without locks) should be relatively easy -- assuming we're willing to accept races between printing messages, and new messages coming in -- but I don't think that the Buffered handler can be made safe without locking at least the flushing part). An it's not just that the messages may be slightly wonky. I think it wouldn't be hard to crash the process by spewing log messages from multiple threads (and we're pretty good at spewing :P).
>
> So, what I'd recommend is to drop the thread-safe flag, and make the locking strategy the responsibility of the individual log handler. When writing to an unbuffered stream, we wouldn't need any locks at all. The circular handler could either lock into that or, if you're into that sort of thing, use atomics to make enqueueing lock-free. The buffered handler could either use a buffered ostream, wrapped in a mutex, or do something similar to the circular one, while locking only the flushing code.

Yup that makes sense to me. I'll put up a patch for that tomorrow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127922