petpav01 updated this revision to Diff 118375.
petpav01 added a comment.
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`.
https://reviews.llvm.org/D38394
Files:
include/lldb/Utility/DataExtractor.h
source/Core/DumpDataExtractor.cpp
source/Utility/DataExtractor.cpp
unittests/Core/DataExtractorTest.cpp
Index: unittests/Core/DataExtractorTest.cpp
===================================================================
--- unittests/Core/DataExtractorTest.cpp
+++ unittests/Core/DataExtractorTest.cpp
@@ -49,3 +49,120 @@
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 with the minimum allowed byte size.
+ 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 with the minimum allowed byte size.
+ offset = 0;
+ EXPECT_EQ(0x01, LE.GetMaxS64(&offset, 1));
+ EXPECT_EQ(1U, offset);
+ offset = 0;
+ EXPECT_EQ(0x01, BE.GetMaxS64(&offset, 1));
+ EXPECT_EQ(1U, 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);
+}
+
+TEST(DataExtractorTest, GetMaxU64_unchecked) {
+ 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 with the minimum allowed byte size.
+ offset = 0;
+ EXPECT_EQ(0x01U, LE.GetMaxU64_unchecked(&offset, 1));
+ EXPECT_EQ(1U, offset);
+ offset = 0;
+ EXPECT_EQ(0x01U, BE.GetMaxU64_unchecked(&offset, 1));
+ EXPECT_EQ(1U, offset);
+
+ // Check with a non-zero offset.
+ offset = 1;
+ EXPECT_EQ(0x0302U, LE.GetMaxU64_unchecked(&offset, 2));
+ EXPECT_EQ(3U, offset);
+ offset = 1;
+ EXPECT_EQ(0x0203U, BE.GetMaxU64_unchecked(&offset, 2));
+ EXPECT_EQ(3U, offset);
+
+ // Check with the byte size not being a multiple of 2.
+ offset = 0;
+ EXPECT_EQ(0x07060504030201U, LE.GetMaxU64_unchecked(&offset, 7));
+ EXPECT_EQ(7U, offset);
+ offset = 0;
+ EXPECT_EQ(0x01020304050607U, BE.GetMaxU64_unchecked(&offset, 7));
+ EXPECT_EQ(7U, offset);
+
+ // Check with the maximum allowed byte size.
+ offset = 0;
+ EXPECT_EQ(0x0807060504030201U, LE.GetMaxU64_unchecked(&offset, 8));
+ EXPECT_EQ(8U, offset);
+ offset = 0;
+ EXPECT_EQ(0x0102030405060708U, BE.GetMaxU64_unchecked(&offset, 8));
+ EXPECT_EQ(8U, offset);
+}
Index: source/Utility/DataExtractor.cpp
===================================================================
--- source/Utility/DataExtractor.cpp
+++ source/Utility/DataExtractor.cpp
@@ -17,6 +17,7 @@
#include "lldb/Utility/DataBuffer.h"
#include "lldb/Utility/DataBufferHeap.h"
#include "lldb/Utility/Endian.h"
+#include "lldb/Utility/LLDBAssert.h"
#include "lldb/Utility/Log.h"
#include "lldb/Utility/Stream.h"
#include "lldb/Utility/StreamString.h"
@@ -105,6 +106,20 @@
return llvm::ByteSwap_64(value);
}
+static inline uint64_t ReadMaxInt64(const uint8_t *data, size_t byte_size,
+ ByteOrder byte_order) {
+ uint64_t res = 0;
+ if (byte_order == eByteOrderBig)
+ for (size_t i = 0; i < byte_size; ++i)
+ res = (res << 8) | data[i];
+ else {
+ assert(byte_order == eByteOrderLittle);
+ for (size_t i = 0; i < byte_size; ++i)
+ res = (res << 8) | data[byte_size - 1 - i];
+ }
+ return res;
+}
+
DataExtractor::DataExtractor()
: m_start(nullptr), m_end(nullptr),
m_byte_order(endian::InlHostByteOrder()), m_addr_size(sizeof(void *)),
@@ -551,107 +566,59 @@
return nullptr;
}
-//----------------------------------------------------------------------
-// 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 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;
+ lldbassert(byte_size > 0 && byte_size <= 4 && "GetMaxU32 invalid byte_size!");
+ 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 {
+ lldbassert(byte_size > 0 && byte_size <= 8 && "GetMaxU64 invalid byte_size!");
+ 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;
+ return ReadMaxInt64(data, byte_size, m_byte_order);
+ }
}
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;
+ default: {
+ uint64_t res = ReadMaxInt64(&m_start[*offset_ptr], byte_size, m_byte_order);
+ *offset_ptr += byte_size;
+ return res;
+ }
}
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;
- }
- return 0;
+int64_t DataExtractor::GetMaxS64(offset_t *offset_ptr, size_t byte_size) const {
+ 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,
Index: source/Core/DumpDataExtractor.cpp
===================================================================
--- source/Core/DumpDataExtractor.cpp
+++ source/Core/DumpDataExtractor.cpp
@@ -272,6 +272,13 @@
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)
Index: include/lldb/Utility/DataExtractor.h
===================================================================
--- include/lldb/Utility/DataExtractor.h
+++ include/lldb/Utility/DataExtractor.h
@@ -513,10 +513,8 @@
///
/// Extract a single integer value and update the offset pointed to
/// by \a offset_ptr. The size of the extracted integer is specified
- /// by the \a byte_size argument. \a byte_size should have a value
- /// >= 1 and <= 4 since the return value is only 32 bits wide. Any
- /// \a byte_size values less than 1 or greater than 4 will result in
- /// nothing being extracted, and zero being returned.
+ /// by the \a byte_size argument. \a byte_size must have a value
+ /// >= 1 and <= 4 since the return value is only 32 bits wide.
///
/// @param[in,out] offset_ptr
/// A pointer to an offset within the data that will be advanced
@@ -539,11 +537,9 @@
///
/// Extract a single unsigned integer value and update the offset
/// pointed to by \a offset_ptr. The size of the extracted integer
- /// is specified by the \a byte_size argument. \a byte_size should
+ /// is specified by the \a byte_size argument. \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. Any
- /// \a byte_size values less than 1 or greater than 8 will result in
- /// nothing being extracted, and zero being returned.
+ /// to eight since the return value is 64 bits wide.
///
/// @param[in,out] offset_ptr
/// A pointer to an offset within the data that will be advanced
@@ -570,10 +566,9 @@
/// Extract a single signed integer value (sign extending if required)
/// and update the offset pointed to by \a offset_ptr. The size of
/// the extracted integer is specified by the \a byte_size argument.
- /// \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.
+ /// \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.
///
/// @param[in,out] offset_ptr
/// A pointer to an offset within the data that will be advanced
@@ -589,20 +584,18 @@
/// 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
- /// is specified by the \a byte_size argument. \a byte_size should
+ /// is specified by the \a byte_size argument. \a byte_size must
/// have a value greater than or equal to one and less than or equal
- /// to 8 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.
+ /// to 8 since the return value is 64 bits wide.
///
/// @param[in,out] offset_ptr
/// A pointer to an offset within the data that will be advanced
@@ -641,10 +634,9 @@
/// Extract a single signed integer value (sign extending if required)
/// and update the offset pointed to by \a offset_ptr. The size of
/// the extracted integer is specified by the \a byte_size argument.
- /// \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.
+ /// \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.
///
/// @param[in,out] offset_ptr
/// A pointer to an offset within the data that will be advanced
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
