On Friday 18 December 2009 17:27, David Greene wrote:> > Here's the updated patch. > > Well, that didn't go through right. Here it is again.Argh! Stupid bug fixed. :) Index: include/llvm/Support/circular_raw_ostream.h ==================================================================--- include/llvm/Support/circular_raw_ostream.h (revision 0) +++ include/llvm/Support/circular_raw_ostream.h (revision 0) @@ -0,0 +1,149 @@ +//===-- llvm/Support/circular_raw_ostream.h - Buffered streams --*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This file contains raw_ostream implementations for streams to do circular +// buffering of their output. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_SUPPORT_CIRCULAR_RAW_OSTREAM_H +#define LLVM_SUPPORT_CIRCULAR_RAW_OSTREAM_H + +#include "llvm/Support/raw_ostream.h" + +namespace llvm +{ + /// circular_raw_ostream - A raw_ostream that saves its output in a + /// circular buffer. + /// + class circular_raw_ostream : public raw_ostream { + public: + /// 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; + + private: + /// TheStream - The real stream we output to. We set it to be + /// unbuffered, since we're already doing our own buffering. + /// + raw_ostream *TheStream; + + /// DeleteStream - Do we need to delete TheStream in the + /// destructor? + /// + bool DeleteStream; + + /// BufferSize - The size of the buffer in bytes. + /// + size_t BufferSize; + + /// BufferArray - The actual buffer storage. + /// + char *BufferArray; + + /// Cur - Pointer to the current output point in BufferArray. + /// + char *Cur; + + /// printLog - Dump the contents of the buffer to Stream. + /// + void printLog(void) { + TheStream->write(BufferArray, BufferSize); + Cur = BufferArray; + } + + virtual void write_impl(const char *Ptr, size_t Size); + + /// current_pos - Return the current position within the stream, + /// not counting the bytes currently in the buffer. + virtual uint64_t current_pos() { + // This has the same effect as calling TheStream.current_pos(), + // but that interface is private. + return TheStream->tell() - TheStream->GetNumBytesInBuffer(); + } + + public: + /// circular_raw_ostream - Open the specified file for + /// writing. + /// + /// 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. + /// + 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) + BufferArray = new char[BufferSize]; + Cur = BufferArray; + setStream(Stream, Delete); + } + explicit circular_raw_ostream() + : raw_ostream(/*unbuffered*/true), + TheStream(0), + DeleteStream(PRESERVE_STREAM), + BufferArray(0) { + Cur = BufferArray; + } + + ~circular_raw_ostream() { + flush(); + dumpLog(); + releaseStream(); + delete[] BufferArray; + } + + void setStream(raw_ostream &Stream, bool Delete = false) { + releaseStream(); + + TheStream = &Stream; + DeleteStream = Delete; + + if (BufferSize > 0) { + // This circular_raw_ostream will do its own buffering and it + // doesn't need or want TheStream to do another layer of + // buffering underneath. Tell TheStream not to do its own + // buffering. + TheStream->SetUnbuffered(); + } + } + + /// dumpLog - Force output of the buffer along with a small + /// header. + /// + void dumpLog(void); + + private: + 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(); + } + }; +} // end llvm namespace + + +#endif Index: lib/Support/circular_raw_ostream.cpp ==================================================================--- lib/Support/circular_raw_ostream.cpp (revision 0) +++ lib/Support/circular_raw_ostream.cpp (revision 0) @@ -0,0 +1,47 @@ +//===- circulat_raw_ostream.cpp - Implement the circular_raw_ostream class -===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This implements support for circular buffered streams. +// +//===----------------------------------------------------------------------===// + +#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) { + // Write into the buffer, wrapping if necessary. + while (Size > 0) { + unsigned Bytes = std::min(Size, BufferSize - (Cur - BufferArray)); + memcpy(Cur, Ptr, Bytes); + Size -= Bytes; + Cur += Bytes; + if (Cur == BufferArray + BufferSize) + // Reset the output pointer to the start of the buffer. + Cur = BufferArray; + } + } + else { + TheStream->write(Ptr, Size); + } +} + +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(); + } +}
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." 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?> + /// 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.> + /// 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.> > + /// 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])?> + /// 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.> + 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.> + 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 >.> + > +#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.> +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? -Chris
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 usingAny 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