labath added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127922

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to