Fix dumping of characters with non-standard sizes

[petpav01]

Summary

• Prevent dumping of characters in DumpDataExtractor() with item_byte_size bigger than 8 bytes. This case is not supported by the code and results in a crash because the code calls DataExtractor::GetMaxU64Bitfield() -> GetMaxU64() that asserts for byte size > 8 bytes.
• Teach DataExtractor::GetMaxU64(), GetMaxU32() and GetMaxS64() how to handle byte sizes that are not a multiple of 2. This allows DumpDataExtractor() to dump characters and booleans with item_byte_size in the interval of [1, 8] bytes. Values that are not a multiple of 2 would previously result in a crash because they were not handled by GetMaxU64().

Examples of two commands that previously resulted in a crash when debugging an AArch64 target, and their new behaviour:

(lldb) register read --format character v0
      v0 = error: unsupported byte size (16) for char format
(lldb) memory read --format boolean --size 7 $sp
0x7ffffffd70: false
0x7ffffffd77: false
[...]

[jingham]

This looks fine to me. I'd give Greg a little time to weigh in, this is much more his code than mine. But I don't see any problem with this, and thanks for the tests!

source/Core/DumpDataExtractor.cpp

275–281

Should this consume the weird input we couldn't print? I actually don't have a good feel for which would be better.

source/Utility/DataExtractor.cpp

566

This is trivial, and you didn't change what was there, but this message makes it sound like this is just something we haven't gotten to yet. It's really "You passed in an illegal byte size"... Might be clearer if the message said that.

[petpav01]

Thank you for the initial review.

source/Core/DumpDataExtractor.cpp

275–281

The behaviour implemented in DumpDataExtractor() for other formats, such as eFormatBoolean or eFormatComplexInteger, is to report an error and do not advance the offset. The approach that the patch takes is to make eFormatChar (and its variants) consistent with this behaviour.

source/Utility/DataExtractor.cpp

566

I was not sure what is the expected behaviour when the input byte_size exceeds the size of the return type of each of these GetMax...() methods. The current behaviour is to assert this situation but comments describing the methods (in both DataExtractor.cpp and DataExtractor.h) say that nothing should get extracted in these cases and zero is returned.

Maybe the patch should go a bit further and clean this up as follows:

• Remove duplicated comments in DataExtractor.cpp for DataExtractor::GetMaxU32() and GetMaxU64() and keep only their Doxygen versions in DataExtractor.h.
• Update comments in DataExtractor.h for DataExtractor::GetMaxU32(), GetMaxU64(), GetMaxS64(), GetMaxU64Bitfield() and GetMaxS64Bitfield() to match the current implementation.
• Change assertion text in DataExtractor::GetMaxU32() and GetMaxU64() from "unhandled case" to "invalid byte size".

Does this sound reasonable?

[jingham]

See inlined comments.

source/Core/DumpDataExtractor.cpp

275–281

The doc's say an error in the size "will result in nothing being extracted". That seems like a roundabout way of saying that offset won't be advanced, but I probably should have figured it out from there. Thanks for checking on the error behavior of the other dumpers! You are doing the right thing.

source/Utility/DataExtractor.cpp

566

The released versions of lldb - at least the ones Apple releases - have asserts disabled. This isn't unique to lldb, clang does the same thing.

I do my day-to-day debugging using a TOT build with asserts enabled, and we run the testsuite that way so the asserts catch errors at this stage. But for the general public, the function will behave as described. It would be great to remove the duplicated docs - that's just begging for one or the other to get out of date. But the descriptions are functionally correct. And then changing the text to "invalid byte size" also seems good to me.

[zturner]

source/Utility/DataExtractor.cpp

566

Being pedantic, this *is* a functionality change. Previously, we would assert on a size of 3 or 0, with this change we will allow those cases through.

[petpav01]

source/Utility/DataExtractor.cpp

566

To explain myself better, what I was thinking about is that e.g. GetMaxU64() should have part:

"\a byte_size should have a value greater than or equal to one and less than or equal to eight since the return value is 64 bits wide. Any \a byte_size values less than 1 or greater than 8 will result in nothing being extracted, and zero being returned."

changed to:

"\a byte_size must have a value greater than or equal to one and less than or equal to eight since the return value is 64 bits wide. The behaviour is undefined for any \a byte_size values less than 1 or greater than 8."

This way the comment provides information that does not depend on whether assertions are enabled or not. The behaviour for byte_size > 8 is said to be undefined in the updated description because it either results in an assertion failure or some undefined behaviour if asserts are disabled.

If the behaviour for byte_size > 4/8 with assertions disabled should actually be that these methods still return 0 and do not advance the offset then the patch has two bugs:

• The general case added in GetMaxU64() is not correct. It returns an unexpected value for byte_size > 8 and advances the offset.
• GetMaxU32() needs to have if (byte_size > 4) return 0; added before it calls GetMaxU64() to avoid the same problem for any byte_size > 4.

An additional thing is that the patch causes that byte_size == 0 is now fully valid and does not assert. This might not be the best idea given that the current descriptions say that byte_size values should be in interval [1, 4/8]. I will add the assertion for byte_size == 0 back in the updated patch so the changes affect/enable only byte_size in range [1, 4/8] (which are clear to be valid) and the zero corner case has its behaviour unchanged.

[clayborg]

Looks good. Would be nice to add support for byte sizes of 3, 5 and 7 to the unchecked version as noted in inline comments, or remove the function if no one is using this function. Just a few quick fixes and this will be good to go.

source/Utility/DataExtractor.cpp

566

use lldbassert if the function will function correctly with the assert removed. I know the previous code was always asserting, but we should change it to use lldbassert to make sure we don't crash the debugger in release builds.

626

Shouldn't we handle the 3, 5 and 7 sizes here too?

unittests/Core/DataExtractorTest.cpp

134

add a test for the unchecked version here?

[petpav01]

Updated patch contains the following changes:

• Remove comments for DataExtractor::GetMaxU32() and GetMaxU64() from DataExtractor.cpp and keep only the Doxygen ones in the header file.
• Restore assertion for byte_size == 0 in GetMaxU32() and GetMaxU64(), change the assert text from "unhandled case" to "invalid byte_size" and replace assert() by lldbassert().
• Update Doxygen documentation for GetMaxU32(), GetMaxU64(), GetMaxS64(), GetMaxU64Bitfield() and GetMaxS64Bitfield() to say that byte_size must be in interval [1,4/8] and remove that the methods return 0 if byte_size is bigger than 4/8 because that no longer holds. The patch retains the behaviour that LLDB does not crash in such cases but the returned value can be arbitrary. Note: This is something that I am not certain if I addressed properly. It seems to me this should be ok because the code now uses lldbassert() and so there will be always some error that something went wrong. An alternative is to add extra code that checks for byte_size > 4/8 and returns 0 in such cases. Please let me know if that would be preferred.
• Enable GetMaxU64_unchecked() to also handle any byte_size in range [1,8] and add testing for this method in DataExtractorTest.cpp.

[clayborg]

Looks good

[petpav01]

Thanks, will commit shortly.

[petpav01]

Landed as r315444. Closing manually because I forgot to include "Differential Revision: ..." in the commit message.

include/lldb/Utility/DataExtractor.h

public:
   ///
   /// @param[in] byte_size
   ///     The size in byte of the integer to extract.
   ///
   /// @return
   ///     The sign extended signed integer value that was extracted,
   ///     or zero on failure.
   //------------------------------------------------------------------
-  int64_t GetMaxS64(lldb::offset_t *offset_ptr, size_t size) const;
+  int64_t GetMaxS64(lldb::offset_t *offset_ptr, size_t byte_size) const;
 
   //------------------------------------------------------------------
   /// Extract an unsigned integer of size \a byte_size from \a
   /// *offset_ptr, then extract the bitfield from this value if
   /// \a bitfield_bit_size is non-zero.
   ///
   /// Extract a single unsigned integer value and update the offset
   /// pointed to by \a offset_ptr. The size of the extracted integer

source/Core/DumpDataExtractor.cpp

case eFormatBytesWithASCII:
       // is being dumped in a group (item_byte_size is more than 1).
       if (item_byte_size > 1)
         s->PutChar(' ');
       break;
 
     case eFormatChar:
     case eFormatCharPrintable:
     case eFormatCharArray: {
+      // Reject invalid item_byte_size.
+      if (item_byte_size > 8) {
+        s->Printf("error: unsupported byte size (%" PRIu64 ") for char format",
+                  (uint64_t)item_byte_size);
+        return offset;
+      }
+
       // If we are only printing one character surround it with single
       // quotes
       if (item_count == 1 && item_format == eFormatChar)
         s->PutChar('\'');
 
       const uint64_t ch = DE.GetMaxU64Bitfield(&offset, item_byte_size,
                                                item_bit_size, item_bit_offset);
       if (isprint(ch))

source/Utility/DataExtractor.cpp

 // a value between 1 and 4 since the return value is only 32 bits
 // wide. Any "byte_size" values less than 1 or greater than 4 will
 // result in nothing being extracted, and zero being returned.
 //
 // RETURNS the integer value that was extracted, or zero on failure.
 //----------------------------------------------------------------------
 uint32_t DataExtractor::GetMaxU32(offset_t *offset_ptr,
                                   size_t byte_size) const {
-  switch (byte_size) {
-  case 1:
-    return GetU8(offset_ptr);
-    break;
-  case 2:
-    return GetU16(offset_ptr);
-    break;
-  case 4:
-    return GetU32(offset_ptr);
-    break;
-  default:
-    assert(false && "GetMaxU32 unhandled case!");
-    break;
-  }
-  return 0;
+  assert(byte_size <= 4 && "GetMaxU32 unhandled case!");
+  return GetMaxU64(offset_ptr, byte_size);
 }
 
 //----------------------------------------------------------------------
 // Extract a single integer value from the data and update the offset
 // pointed to by "offset_ptr". The size of the extracted integer
 // is specified by the "byte_size" argument. "byte_size" should have
 // a value >= 1 and <= 8 since the return value is only 64 bits
 // wide. Any "byte_size" values less than 1 or greater than 8 will
 // result in nothing being extracted, and zero being returned.
 //
 // RETURNS the integer value that was extracted, or zero on failure.
 //----------------------------------------------------------------------
-uint64_t DataExtractor::GetMaxU64(offset_t *offset_ptr, size_t size) const {
-  switch (size) {
+uint64_t DataExtractor::GetMaxU64(offset_t *offset_ptr,
+                                  size_t byte_size) const {
+  assert(byte_size <= 8 && "GetMax64 unhandled case!");
+  switch (byte_size) {
   case 1:
     return GetU8(offset_ptr);
-    break;
   case 2:
     return GetU16(offset_ptr);
-    break;
   case 4:
     return GetU32(offset_ptr);
-    break;
   case 8:
     return GetU64(offset_ptr);
-    break;
-  default:
-    assert(false && "GetMax64 unhandled case!");
-    break;
+  default: {
+    // General case.
+    const uint8_t *data =
+        static_cast<const uint8_t *>(GetData(offset_ptr, byte_size));
+    if (data == nullptr)
+      return 0;
+
+    uint64_t res = 0;
+    if (m_byte_order == eByteOrderBig)
+      for (size_t i = 0; i < byte_size; ++i)
+        res = (res << 8) | data[i];
+    else {
+      assert(m_byte_order == eByteOrderLittle);
+      for (size_t i = 0; i < byte_size; ++i)
+        res = (res << 8) | data[byte_size - 1 - i];
+    }
+    return res;
+  }
   }
   return 0;
 }
 
 uint64_t DataExtractor::GetMaxU64_unchecked(offset_t *offset_ptr,
-                                            size_t size) const {
-  switch (size) {
+                                            size_t byte_size) const {
+  switch (byte_size) {
   case 1:
     return GetU8_unchecked(offset_ptr);
-    break;
   case 2:
     return GetU16_unchecked(offset_ptr);
-    break;
   case 4:
     return GetU32_unchecked(offset_ptr);
-    break;
   case 8:
     return GetU64_unchecked(offset_ptr);
-    break;
   default:
-    assert(false && "GetMax64 unhandled case!");
-    break;
+    llvm_unreachable("GetMax64_unchecked unhandled case!");
   }
   return 0;
 }
 
-int64_t DataExtractor::GetMaxS64(offset_t *offset_ptr, size_t size) const {
-  switch (size) {
-  case 1:
-    return (int8_t)GetU8(offset_ptr);
-    break;
-  case 2:
-    return (int16_t)GetU16(offset_ptr);
-    break;
-  case 4:
-    return (int32_t)GetU32(offset_ptr);
-    break;
-  case 8:
-    return (int64_t)GetU64(offset_ptr);
-    break;
-  default:
-    assert(false && "GetMax64 unhandled case!");
-    break;
-  }
+int64_t DataExtractor::GetMaxS64(offset_t *offset_ptr, size_t byte_size) const {
+  if (byte_size == 0)
     return 0;
+
+  uint64_t u64 = GetMaxU64(offset_ptr, byte_size);
+  return llvm::SignExtend64(u64, 8 * byte_size);
 }
 
 uint64_t DataExtractor::GetMaxU64Bitfield(offset_t *offset_ptr, size_t size,
                                           uint32_t bitfield_bit_size,
                                           uint32_t bitfield_bit_offset) const {
   uint64_t uval64 = GetMaxU64(offset_ptr, size);
   if (bitfield_bit_size > 0) {
     int32_t lsbcount = bitfield_bit_offset;

unittests/Core/DataExtractorTest.cpp

TEST(DataExtractorTest, PeekData) {
 
   EXPECT_EQ(buffer + 2, E.PeekData(2, 0));
   EXPECT_EQ(buffer + 2, E.PeekData(2, 2));
   EXPECT_EQ(nullptr, E.PeekData(2, 3));
 
   EXPECT_EQ(buffer + 4, E.PeekData(4, 0));
   EXPECT_EQ(nullptr, E.PeekData(4, 1));
 }
+
+TEST(DataExtractorTest, GetMaxU64) {
+  uint8_t buffer[] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08};
+  DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+                   sizeof(void *));
+  DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig, sizeof(void *));
+
+  lldb::offset_t offset;
+
+  // Check cases at the lower end with the byte size equal to 0 and 1.
+  offset = 0;
+  EXPECT_EQ(0U, LE.GetMaxU64(&offset, 0));
+  EXPECT_EQ(0U, offset);
+  offset = 0;
+  EXPECT_EQ(0U, BE.GetMaxU64(&offset, 0));
+  EXPECT_EQ(0U, offset);
+
+  offset = 0;
+  EXPECT_EQ(0x01U, LE.GetMaxU64(&offset, 1));
+  EXPECT_EQ(1U, offset);
+  offset = 0;
+  EXPECT_EQ(0x01U, BE.GetMaxU64(&offset, 1));
+  EXPECT_EQ(1U, offset);
+
+  // Check with a non-zero offset.
+  offset = 1;
+  EXPECT_EQ(0x0302U, LE.GetMaxU64(&offset, 2));
+  EXPECT_EQ(3U, offset);
+  offset = 1;
+  EXPECT_EQ(0x0203U, BE.GetMaxU64(&offset, 2));
+  EXPECT_EQ(3U, offset);
+
+  // Check with the byte size not being a multiple of 2.
+  offset = 0;
+  EXPECT_EQ(0x07060504030201U, LE.GetMaxU64(&offset, 7));
+  EXPECT_EQ(7U, offset);
+  offset = 0;
+  EXPECT_EQ(0x01020304050607U, BE.GetMaxU64(&offset, 7));
+  EXPECT_EQ(7U, offset);
+
+  // Check with the maximum allowed byte size.
+  offset = 0;
+  EXPECT_EQ(0x0807060504030201U, LE.GetMaxU64(&offset, 8));
+  EXPECT_EQ(8U, offset);
+  offset = 0;
+  EXPECT_EQ(0x0102030405060708U, BE.GetMaxU64(&offset, 8));
+  EXPECT_EQ(8U, offset);
+}
+
+TEST(DataExtractorTest, GetMaxS64) {
+  uint8_t buffer[] = {0x01, 0x02, 0x83, 0x04, 0x05, 0x06, 0x07, 0x08};
+  DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+                   sizeof(void *));
+  DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig, sizeof(void *));
+
+  lldb::offset_t offset;
+
+  // Check a trivial case with the byte size equal to 0.
+  offset = 0;
+  EXPECT_EQ(0, LE.GetMaxS64(&offset, 0));
+  EXPECT_EQ(0U, offset);
+  offset = 0;
+  EXPECT_EQ(0, BE.GetMaxS64(&offset, 0));
+  EXPECT_EQ(0U, offset);
+
+  // Check that sign extension works correctly.
+  offset = 0;
+  int64_t value = LE.GetMaxS64(&offset, 3);
+  EXPECT_EQ(0xffffffffff830201U, *reinterpret_cast<uint64_t *>(&value));
+  EXPECT_EQ(3U, offset);
+  offset = 2;
+  value = BE.GetMaxS64(&offset, 3);
+  EXPECT_EQ(0xffffffffff830405U, *reinterpret_cast<uint64_t *>(&value));
+  EXPECT_EQ(5U, offset);
+
+  // Check with the maximum allowed byte size.
+  offset = 0;
+  EXPECT_EQ(0x0807060504830201, LE.GetMaxS64(&offset, 8));
+  EXPECT_EQ(8U, offset);
+  offset = 0;
+  EXPECT_EQ(0x0102830405060708, BE.GetMaxS64(&offset, 8));
+  EXPECT_EQ(8U, offset);
+}