This is an archive of the discontinued LLVM Phabricator instance.

Convert Log class to llvm streams
ClosedPublic

Authored by labath on Feb 6 2017, 6:06 PM.

Details

Summary

This converts LLDB's logging to use llvm streams instead of
lldb_private::Stream and friends. The changes are mostly
straight-forward and amount to s/lldb_private::Stream/llvm::raw_ostream.

The part worth calling out is the rewrite of the StreamCallback class.
Previously this class contained a per-thread buffer of data written. I
assume this had something to do with it trying to make sure each log
line is delivered as a single event, instead of multiple (possibly
interleaved) events. However, this is no longer relevant as the Log
class already writes things to a temporary buffer and then delivers the
message as a single "write", so I have just removed the code in
question.

Event Timeline

labath created this revision.Feb 6 2017, 6:06 PM
zturner edited edge metadata.Feb 6 2017, 7:10 PM

Where is the logging callback used? It seems rather odd that we would need to notify someone every time a log message was generated.

source/Core/StreamCallback.cpp
22

I find it rather odd that this was hardcoding big endian. Was the endianness here important for some reason?

labath added a comment.Feb 6 2017, 7:19 PM

The log callback gets passed down from the SB API (in the SBDebugger constructor, no less). My best guess is that it is (was?) used to display the debugger log in some IDE window.

source/Core/StreamCallback.cpp
22

I think that was there just because you needed to specify some value. As we were always printing strings, it did not matter anyway.

labath added a comment.Feb 8 2017, 9:15 AM

Ping. Any thoughts on this?

clayborg accepted this revision.Feb 8 2017, 9:38 AM

Looks good as long as if we type two log enable commands like:

(lldb) log enable -f /tmp/a.txt lldb process
(lldb) log enable -f /tmp/a.txt lldb api

share the same log stream (we don't open /tmp/a.txt twice) if is still open and available.

source/Core/StreamCallback.cpp
22

Pavel is correct, this is because streams can be put into binary mode and that you can use Stream::Put32(uint32_t) which will write a number if in non-binary mode, or endian correct bytes if in binary mode. Logs didn't ever use the binary feature so we just set it to defaults.

This revision is now accepted and ready to land.Feb 8 2017, 9:38 AM
labath added a comment.Feb 8 2017, 4:55 PM

Thank you for the review. I'll submit this tomorrow.

Looks good as long as if we type two log enable commands like:

(lldb) log enable -f /tmp/a.txt lldb process
(lldb) log enable -f /tmp/a.txt lldb api

Yes, the log file still gets shared. I am not changing that.

@zturner:
Aside from the thread stuff, nothing seems particularly risky about this change. Did the thread-local stuff you removed have anything to do with the -t option to the log command?

Well... the thread-safe logging would make that unnecessary, but with the current temporary-buffer implementation you don't need that even without threadsafe logging. This makes be believe the logging callback was written before we had the temporary buffer logic in place, but I haven't dug through the history to verify that.

This revision was automatically updated to reflect the committed changes.