[LLVMdev] [PATCH] Circular Buffered Debug Stream

[David Greene]

On Friday 18 December 2009 19:47, Chris Lattner wrote:
> On Dec 18, 2009, at 3:46 PM, David Greene wrote:
> > +  /// circular_raw_ostream - A raw_ostream that saves its output in a
> > +  /// circular buffer.
>
> A better description would be "which *can* save its data to a circular
> buffer, or can pass it through directly to an underlying stream if
> specified with a buffer of zero."

Make sense.

> When it is buffering, what causes it to flush?  Your current model
> requires "printLog" which is not an obvious name for this, would it be
> better to do it on destruction?  How about using something like
> flush_to_underlying_stream() or something like that?

Currently, it uses a signal handler.  It's important that one gets output
on a SIGTERM, SIGSGEV and other such things and I don't know that object
desctruction is guaranteed in those cases.

Eventually we'll want to dump on SIGUSER1 as well but there's no generic 
libSystem hook for that yet.

> > +    /// DELETE_STREAM - Tell the destructor to delete the held
> > stream.
> > +    ///
> > +    static const bool DELETE_STREAM = true;
> > +
> > +    /// PRESERVE_STREAM - Tell the destructor to not delete the held
> > +    /// stream.
> > +    ///
> > +    static const bool PRESERVE_STREAM = false;
>
> I don't find these to be very clear names.  How about giving them a
> name involving 'ownership' like TAKES_OWNERSHIP?  "preservation" isn't
> really the same as "not deleting when the stream is destroyed" to me.

That's a good idea.

I just took these from what Dan (I think) did in formatted_raw_ostream.
I'll change them here and rename DeleteStream to OwnsStream as well.

> > +    /// TheStream - The real stream we output to. We set it to be
> > +    /// unbuffered, since we're already doing our own buffering.
> > +    ///
> > +    raw_ostream *TheStream;
>
> Now that I understand the model :) I don't see why you need to fiddle
> with the underlying buffering of TheStream.  In the dbgs() use case,
> the underlying stream will be errs() which is already unbuffered.

Might circular_raw_ostream be used with other streams at some point?

> > +    /// printLog - Dump the contents of the buffer to Stream.
> > +    ///
> > +    void printLog(void) {
> > +      TheStream->write(BufferArray, BufferSize);
> > +      Cur = BufferArray;
> > +    }
>
> If the buffer has circled around, won't this print it out-of-order?
> If the current pointer is 5 and the length is 10, it seems like this
> should do a write([5..9]) then write([0..4])?

Aha!  Good catch.  This worked differently in our implementation here.  I'll
fix it.

> > +    /// current_pos - Return the current position within the stream,
> > +    /// not counting the bytes currently in the buffer.
> > +    virtual uint64_t current_pos() {
>
> This is now const on mainline.

Ok.

> > +  public:
> > +    /// circular_raw_ostream - Open the specified file for
> > +    /// writing.
>
> This is not a file, please read the comments you're copying :)

:)

> > +    ///
> > +    /// As a side effect, if BuffSize is nonzero, the given Stream is
> > +    /// set to be Unbuffered. This is because circular_raw_ostream
> > +    /// does its own buffering, so it doesn't want another layer of
> > +    /// buffering to be happening underneath it.
> > +    ///
>
> Again, I don't think this should happen anymore.  You should add some
> comment about what Delete does, and change it to not use 'false' as I
> asked for before.

I've cleaned this up, but see my question about usage with other streams.

> > +    circular_raw_ostream(raw_ostream &Stream, size_t BuffSize = 8192,
> > +                         bool Delete = false)
> > +        : raw_ostream(/*unbuffered*/true),
> > +            TheStream(0),
> > +            DeleteStream(PRESERVE_STREAM),
> > +            BufferSize(BuffSize),
> > +            BufferArray(0) {
> > +      if (BufferSize > 0)
>
> Please change all these predicates to BufferSize != 0 instead of using

Any particular reason?  I'll do it but I'm just curious.  It seems that if
these things get changed to signed someday it could cause trouble.

> > +#include "llvm/Support/circular_raw_ostream.h"
> > +
> > +#include <algorithm>
> > +
> > +using namespace llvm;
> > +
> > +void circular_raw_ostream::write_impl(const char *Ptr, size_t Size) {
> > +  if (BufferSize > 0) {
>
> Please use:
>
> if (BufferSize == 0) {
>    .. write ..
>    return;
> }
>
> to reduce nesting.

Ok.

> > +void circular_raw_ostream::dumpLog(void) {
> > +  if (BufferSize > 0) {
> > +    // Write out the buffer
> > +    const char *msg = "*** Log Output ***\n";
> > +    int num = std::strlen(msg);
> > +
> > +    TheStream->write(msg, num);
> > +    printLog();
> > +  }
> > +}
>
> I'm still unclear about this and printLog.  The "Log Output" prefix is
> confusing to me, because this isn't a log file.  Maybe just remove the
> prefix?

I wanted something to signify in the output where the debug dump starts.
That's for that case where some stuff might still get output through errs().
I anticipate that while I'll change most uses of errs() to dbgs(), we
probably don't want to change all of them.

I agree the naming here is poor.  I'll fix that.

                                   -Dave