[lldb] Don't rely on unsigned integer wrapping in PutRawBytes and PutBytesAsRawHex8

[JDevlieghere]

Summary

I was looking at Stream::PutRawBytes and thought I spotted a bug because both looks are using i < src_len as the loop condition despite them iterating "in different directions". On closer inspection, the existing code is correct, because it relies on well-defined unsigned integer wrapping of size_t. Correct doesn't mean readable, so this patch changes the loop condition to compare against 0 when decrementing i while still covering the edge case of src_len potentially being 0 itself.

[JDevlieghere]

lldb/source/Utility/Stream.cpp

360

Intentional newline for consistency with Stream::PutRawBytes above.

[mib]

LGTM

[shafik]

Do we have a test that covers this edge case?

[JDevlieghere]

• Add unit test for edge case

[labath]

Another option would be to go full-STL and do something like

ArrayRef<uint8_t> data(ptr, len);
if (reverse)
  for (uint8_t byte: llvm::reverse(data))
...

lldb/source/Utility/Stream.cpp

size_t Stream::PutRawBytes(const void *s, size_t src_len,
   const uint8_t *src = static_cast<const uint8_t *>(s);
   bool binary_was_set = m_flags.Test(eBinary);
   if (!binary_was_set)
     m_flags.Set(eBinary);
   if (src_byte_order == dst_byte_order) {
     for (size_t i = 0; i < src_len; ++i)
       _PutHex8(src[i], false);
   } else {
-    for (size_t i = src_len - 1; i < src_len; --i)
-      _PutHex8(src[i], false);
+    for (size_t i = src_len; i > 0; --i)
+      _PutHex8(src[i - 1], false);
   }
   if (!binary_was_set)
     m_flags.Clear(eBinary);
 
   return *delta;
 }
 
 size_t Stream::PutBytesAsRawHex8(const void *s, size_t src_len,
                                  ByteOrder src_byte_order,
                                  ByteOrder dst_byte_order) {
   ByteDelta delta(*this);
+
   if (src_byte_order == eByteOrderInvalid)
     src_byte_order = m_byte_order;
 
   if (dst_byte_order == eByteOrderInvalid)
     dst_byte_order = m_byte_order;
 
   const uint8_t *src = static_cast<const uint8_t *>(s);
   bool binary_is_set = m_flags.Test(eBinary);
   m_flags.Clear(eBinary);
   if (src_byte_order == dst_byte_order) {
     for (size_t i = 0; i < src_len; ++i)
       _PutHex8(src[i], false);
   } else {
-    for (size_t i = src_len - 1; i < src_len; --i)
-      _PutHex8(src[i], false);
+    for (size_t i = src_len; i > 0; --i)
+      _PutHex8(src[i - 1], false);
   }
   if (binary_is_set)
     m_flags.Set(eBinary);
 
   return *delta;
 }
 
 size_t Stream::PutStringAsRawHex8(llvm::StringRef s) {

lldb/unittests/Utility/StreamTest.cpp

TEST_F(StreamTest, PutRawBytesToMixedEndian) {
 
   // FIXME: PDP byte order is not actually implemented but Stream just silently
   // prints the value in some random byte order...
 #if 0
   EXPECT_EQ("\x34\x12\x78\x56", TakeValue());
 #endif
 }
 
+TEST_F(StreamTest, PutRawBytesZeroLenght) {
+  uint32_t value = 0x12345678;
+
+  s.PutRawBytes(static_cast<void *>(&value), 0, hostByteOrder,
+                lldb::eByteOrderLittle);
+  EXPECT_EQ(0U, s.GetWrittenBytes());
+
+  s.PutRawBytes(static_cast<void *>(&value), 0, hostByteOrder,
+                lldb::eByteOrderBig);
+  EXPECT_EQ(0U, s.GetWrittenBytes());
+}
+
+TEST_F(StreamTest, PutBytesAsRawHex8ZeroLenght) {
+  uint32_t value = 0x12345678;
+
+  s.PutBytesAsRawHex8(static_cast<void *>(&value), 0, hostByteOrder,
+                      lldb::eByteOrderLittle);
+  EXPECT_EQ(0U, s.GetWrittenBytes());
+
+  s.PutBytesAsRawHex8(static_cast<void *>(&value), 0, hostByteOrder,
+                      lldb::eByteOrderBig);
+  EXPECT_EQ(0U, s.GetWrittenBytes());
+}
+
 // ULEB128 support for binary streams.
 
 TEST_F(BinaryStreamTest, PutULEB128OneByte) {
   auto bytes = s.PutULEB128(0x74ULL);
   EXPECT_EQ(1U, s.GetWrittenBytes());
   EXPECT_EQ("\x74", TakeValue());
   EXPECT_EQ(1U, bytes);
 }