[LLVMdev] [PATCH] Circular Buffered Debug Stream

[David Greene]

On Friday 18 December 2009 12:28, Chris Lattner wrote:

> This is looking a lot better, here are some more comments:
> > +    /// current_pos - Return the current position within the stream,
> > +    /// not counting the bytes currently in the buffer.
> > +    virtual uint64_t current_pos() {
>
> I didn't notice this earlier, but is there any reason for current_pos to be
> non-const in raw_ostream?

Probably not.  I'm going to let someone else fix that, though.

> > +    /// circular_raw_ostream - Open the specified file for
> > +    /// writing. If an error occurs, information about the error is
> > +    /// put into ErrorInfo, and the stream should be immediately
> > +    /// destroyed; the string will be empty if no error occurred.
>
> This comment is out of date, there is no ErrorInfo.

Will remove.

> > +    ///
> > +    /// As a side effect, 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.
> > +    ///
> > +    circular_raw_ostream(raw_ostream &Stream, unsigned BuffSize = 8192,
> > +                         bool Delete = false)
> > +        : raw_ostream(/*unbuffered*/true),
> > +            TheStream(0),
> > +            DeleteStream(PRESERVE_STREAM),
> > +            BufferSize(BuffSize),
> > +            BufferArray(0) {
> > +      if (BufferSize > 0)
> > +        BufferArray = new char[BufferSize];
>
> How about just asserting that BufferSize is not zero?  This stream won't
> work well and doesn't make sense with a buffer of zero.

Sure it does.  In fact it's crucial for making dbgs() work like errs() in
the default case.

> > +    void setStream(raw_ostream &Stream, bool Delete = false) {
> > +      releaseStream();
> > +
> > +      TheStream = &Stream;
> > +      DeleteStream = Delete;
> > +
> > +      if (BufferSize > 0) {
>
> The only time where it makes sense for BufferSize to be zero here is if the
> circular stream was default constructed.  In this case, it seems reasonable
> to allocate a buffer here instead of trying to operate without one.

See above.


> > +    void releaseStream() {
> > +      // Delete the stream if needed. Otherwise, transfer the buffer
> > +      // settings from this raw_ostream back to the underlying stream.
> > +      if (!TheStream)
> > +        return;
> > +      if (DeleteStream)
> > +        delete TheStream;
> > +      else if (BufferSize > 0)
> > +        TheStream->SetBufferSize(BufferSize);
> > +      else
> > +        TheStream->SetUnbuffered();
>
> Again, I don't think an unbuffered circular stream makes a lot of sense
> here. 

See above.  :)

> Another issue is that this is transfering the circular stream's 
> buffer to the underlying stream.  Would it make more sense and would it be
> worth it to save the underlying streams buffer size and restore it in
> releaseStream?

Oh, I think this code is just wrong.  I'll rework it.

> > +void circular_raw_ostream::write_impl(const char *Ptr, size_t Size) {
> > +  if (BufferSize > 0) {
>
> This check can go away.

Nope.  We don't want to pring the "Log Output" banner if we're not buffering
(and delaying output until termination).  Again, we want dbgs() to work like
errs() in the default case and output immediately.

> > +    // Write into the buffer, wrapping if necessary.
> > +    while (Size > 0) {
> > +      while (Size > 0 && Cur != BufferArray + BufferSize) {
> > +        --Size;
> > +        *Cur++ = *Ptr++;
> > +      }
>
> This should use memcpy.

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();
> > +  }
> > +}
>
> Is this to debug the implementation of the stream?  Should this go into the
> tree?

No, this is what gets dumped at program termination.  Remember, this stream
buffers everything you send it, effectively dropping everything except the
last N characters, when N == BufferSize.  Then at program termination it
dumps the buffer.

logs() might be a better name than dbgs() for this.  What do you think?

                              -Dave