[llvm-commits] [llvm] r79862 - in /llvm/trunk: include/llvm/Support/Format.h lib/Support/raw_ostream.cpp unittests/Support/raw_ostream_test.cpp

[Daniel Dunbar]

Author: ddunbar
Date: Sun Aug 23 15:31:39 2009
New Revision: 79862

URL: http://llvm.org/viewvc/llvm-project?rev=79862&view=rev
Log:
Fix off-by-one in llvm::Format::print.
 - This also shortens the Format.h implementation, and uses the print buffer
   fully (it was wasting a character).

 - This manifested as llvm-test failures, because one side effect was that
   raw_ostream would write garbage '\x00' values into the output stream if it
   happened that the string was at the end of the buffer. This meant that grep
   would report 'Binary file matches', which meant the silly pattern matching
   llvm-test eventually does would fail. Cute. :)

Modified:
    llvm/trunk/include/llvm/Support/Format.h
    llvm/trunk/lib/Support/raw_ostream.cpp
    llvm/trunk/unittests/Support/raw_ostream_test.cpp

Modified: llvm/trunk/include/llvm/Support/Format.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Format.h?rev=79862&r1=79861&r2=79862&view=diff

==============================================================================
--- llvm/trunk/include/llvm/Support/Format.h (original)
+++ llvm/trunk/include/llvm/Support/Format.h Sun Aug 23 15:31:39 2009
@@ -36,6 +36,10 @@
 protected:
   const char *Fmt;
   virtual void home(); // Out of line virtual method.
+
+  /// snprint - Call snprintf() for this object, on the given buffer and size.
+  virtual int snprint(char *Buffer, unsigned BufferSize) const = 0;
+
 public:
   format_object_base(const char *fmt) : Fmt(fmt) {}
   virtual ~format_object_base() {}
@@ -43,7 +47,23 @@
   /// print - Format the object into the specified buffer.  On success, this
   /// returns the length of the formatted string.  If the buffer is too small,
   /// this returns a length to retry with, which will be larger than BufferSize.
-  virtual unsigned print(char *Buffer, unsigned BufferSize) const = 0;
+  unsigned print(char *Buffer, unsigned BufferSize) const {
+    assert(BufferSize && "Invalid buffer size!");
+
+    // Print the string, leaving room for the terminating null.
+    int N = snprint(Buffer, BufferSize);
+
+    // VC++ and old GlibC return negative on overflow, just double the size.
+    if (N < 0)
+      return BufferSize*2;
+
+    // Other impls yield number of bytes needed, not including the final '\0'.
+    if (unsigned(N) >= BufferSize)
+      return N+1;
+
+    // Otherwise N is the length of output (not including the final '\0').
+    return N;
+  }
 };
 
 /// format_object1 - This is a templated helper class used by the format
@@ -58,17 +78,8 @@
     : format_object_base(fmt), Val(val) {
   }
 
-  /// print - Format the object into the specified buffer.  On success, this
-  /// returns the length of the formatted string.  If the buffer is too small,
-  /// this returns a length to retry with, which will be larger than BufferSize.
-  virtual unsigned print(char *Buffer, unsigned BufferSize) const {
-    int N = snprintf(Buffer, BufferSize-1, Fmt, Val);
-    if (N < 0)             // VC++ and old GlibC return negative on overflow.
-      return BufferSize*2;
-    if (unsigned(N) >= BufferSize-1)// Other impls yield number of bytes needed.
-      return N+1;
-    // If N is positive and <= BufferSize-1, then the string fit, yay.
-    return N;
+  virtual int snprint(char *Buffer, unsigned BufferSize) const {
+    return snprintf(Buffer, BufferSize, Fmt, Val);
   }
 };
 
@@ -85,17 +96,8 @@
   : format_object_base(fmt), Val1(val1), Val2(val2) {
   }
 
-  /// print - Format the object into the specified buffer.  On success, this
-  /// returns the length of the formatted string.  If the buffer is too small,
-  /// this returns a length to retry with, which will be larger than BufferSize.
-  virtual unsigned print(char *Buffer, unsigned BufferSize) const {
-    int N = snprintf(Buffer, BufferSize-1, Fmt, Val1, Val2);
-    if (N < 0)             // VC++ and old GlibC return negative on overflow.
-      return BufferSize*2;
-    if (unsigned(N) >= BufferSize-1)// Other impls yield number of bytes needed.
-      return N+1;
-    // If N is positive and <= BufferSize-1, then the string fit, yay.
-    return N;
+  virtual int snprint(char *Buffer, unsigned BufferSize) const {
+    return snprintf(Buffer, BufferSize, Fmt, Val1, Val2);
   }
 };
 
@@ -113,17 +115,8 @@
     : format_object_base(fmt), Val1(val1), Val2(val2), Val3(val3) {
   }
 
-  /// print - Format the object into the specified buffer.  On success, this
-  /// returns the length of the formatted string.  If the buffer is too small,
-  /// this returns a length to retry with, which will be larger than BufferSize.
-  virtual unsigned print(char *Buffer, unsigned BufferSize) const {
-    int N = snprintf(Buffer, BufferSize-1, Fmt, Val1, Val2, Val3);
-    if (N < 0)             // VC++ and old GlibC return negative on overflow.
-      return BufferSize*2;
-    if (unsigned(N) >= BufferSize-1)// Other impls yield number of bytes needed.
-      return N+1;
-    // If N is positive and <= BufferSize-1, then the string fit, yay.
-    return N;
+  virtual int snprint(char *Buffer, unsigned BufferSize) const {
+    return snprintf(Buffer, BufferSize, Fmt, Val1, Val2, Val3);
   }
 };
 

Modified: llvm/trunk/lib/Support/raw_ostream.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/raw_ostream.cpp?rev=79862&r1=79861&r2=79862&view=diff

==============================================================================
--- llvm/trunk/lib/Support/raw_ostream.cpp (original)
+++ llvm/trunk/lib/Support/raw_ostream.cpp Sun Aug 23 15:31:39 2009
@@ -253,12 +253,12 @@
   // If we have more than a few bytes left in our output buffer, try
   // formatting directly onto its end.
   size_t NextBufferSize = 127;
-  if (OutBufEnd-OutBufCur > 3) {
-    size_t BufferBytesLeft = OutBufEnd-OutBufCur;
+  size_t BufferBytesLeft = OutBufEnd - OutBufCur;
+  if (BufferBytesLeft > 3) {
     size_t BytesUsed = Fmt.print(OutBufCur, BufferBytesLeft);
     
     // Common case is that we have plenty of space.
-    if (BytesUsed < BufferBytesLeft) {
+    if (BytesUsed <= BufferBytesLeft) {
       OutBufCur += BytesUsed;
       return *this;
     }
@@ -277,11 +277,11 @@
     V.resize(NextBufferSize);
     
     // Try formatting into the SmallVector.
-    size_t BytesUsed = Fmt.print(&V[0], NextBufferSize);
+    size_t BytesUsed = Fmt.print(V.data(), NextBufferSize);
     
     // If BytesUsed fit into the vector, we win.
     if (BytesUsed <= NextBufferSize)
-      return write(&V[0], BytesUsed);
+      return write(V.data(), BytesUsed);
     
     // Otherwise, try again with a new size.
     assert(BytesUsed > NextBufferSize && "Didn't grow buffer!?");

Modified: llvm/trunk/unittests/Support/raw_ostream_test.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/raw_ostream_test.cpp?rev=79862&r1=79861&r2=79862&view=diff

==============================================================================
--- llvm/trunk/unittests/Support/raw_ostream_test.cpp (original)
+++ llvm/trunk/unittests/Support/raw_ostream_test.cpp Sun Aug 23 15:31:39 2009
@@ -8,6 +8,8 @@
 //===----------------------------------------------------------------------===//
 
 #include "gtest/gtest.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/Format.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
@@ -20,6 +22,23 @@
   return res;    
 }
 
+/// printToString - Print the given value to a stream which only has \arg
+/// BytesLeftInBuffer bytes left in the buffer. This is useful for testing edge
+/// cases in the buffer handling logic.
+template<typename T> std::string printToString(const T &Value,
+                                               unsigned BytesLeftInBuffer) {
+  // FIXME: This is relying on internal knowledge of how raw_ostream works to
+  // get the buffer position right.
+  SmallString<256> SVec;
+  assert(BytesLeftInBuffer < 256 && "Invalid buffer count!");
+  llvm::raw_svector_ostream OS(SVec);
+  unsigned StartIndex = 256 - BytesLeftInBuffer;
+  for (unsigned i = 0; i != StartIndex; ++i)
+    OS << '?';
+  OS << Value;
+  return OS.str().substr(StartIndex);
+}
+
 template<typename T> std::string printToStringUnbuffered(const T &Value) {
   std::string res;
   llvm::raw_string_ostream OS(res);
@@ -90,4 +109,12 @@
   EXPECT_EQ("-9223372036854775808", printToStringUnbuffered(INT64_MIN));
 }
 
+TEST(raw_ostreamTest, BufferEdge) {  
+  EXPECT_EQ("1.20", printToString(format("%.2f", 1.2), 1));
+  EXPECT_EQ("1.20", printToString(format("%.2f", 1.2), 2));
+  EXPECT_EQ("1.20", printToString(format("%.2f", 1.2), 3));
+  EXPECT_EQ("1.20", printToString(format("%.2f", 1.2), 4));
+  EXPECT_EQ("1.20", printToString(format("%.2f", 1.2), 10));
+}
+
 }