aykevl updated this revision to Diff 242694.
aykevl set the repository for this revision to rG LLVM Github Monorepo.
aykevl added a comment.
> I do have one question though. Will the DataExtractor actually do something
> reasonable for non-power-of-2 sizes (5,6,7) ? If yes, then great -- if not,
> we should keep the assert as `2 || 4 || 8`.
I checked, and the only places it is actually used is in `GetAddress`,
`GetAddress_unchecked`, and `GetPointer`. All those end up at `GetMaxU64` (or
the unchecked variant) which seems to take care of odd pointer lengths. I have
added a test case to make sure this will keep working.
> Since we are doing the same test all over `m_addr_size >= 1 && m_addr_size <=
> 8` can we just make it a function and avoid the repetition and potential
> erroneous updating later on that does not fix them all?
Right now there are asserts both when constructing/copying(?) the object (5
asserts) and at the place where `m_addr_size` is used (3 asserts). I would
propose picking one place, such as where it is used. That would reduce the
number of asserts to 3 and keep them close together. What do you think?
Sidenote: `GetAddress` and `GetPointer` are the same function but with a
different name. Even the doc comment right before is almost identical. I tried
tracing back where they came from and they can both be traced back to the
initial LLDB checin from Apple
<https://github.com/llvm/llvm-project/commit/30fdc8d841c9d24ac5f3d452b6ece84ee0ac991c>.
Maybe this is worth deduplicating eventually.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73969/new/
https://reviews.llvm.org/D73969
Files:
lldb/source/Utility/DataExtractor.cpp
lldb/unittests/Utility/DataExtractorTest.cpp
Index: lldb/unittests/Utility/DataExtractorTest.cpp
===================================================================
--- lldb/unittests/Utility/DataExtractorTest.cpp
+++ lldb/unittests/Utility/DataExtractorTest.cpp
@@ -112,6 +112,39 @@
EXPECT_EQ(4U, offset);
}
+TEST(DataExtractorTest, UncommonAddressSize) {
+ uint8_t buffer[] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08};
+ DataExtractor E2(buffer, sizeof buffer, lldb::eByteOrderLittle, 2);
+ DataExtractor E5(buffer, sizeof buffer, lldb::eByteOrderLittle, 5);
+ DataExtractor E7(buffer, sizeof buffer, lldb::eByteOrderLittle, 7);
+
+ lldb::offset_t offset;
+
+ // Test 2-byte addresses (for AVR).
+ offset = 0;
+ EXPECT_EQ(0x0201U, E2.GetMaxU64(&offset, 2));
+ EXPECT_EQ(2U, offset);
+ offset = 0;
+ EXPECT_EQ(0x0201U, E2.GetAddress(&offset));
+ EXPECT_EQ(2U, offset);
+
+ // Test 5-byte addresses.
+ offset = 0;
+ EXPECT_EQ(0x030201U, E5.GetMaxU64(&offset, 3));
+ EXPECT_EQ(3U, offset);
+ offset = 3;
+ EXPECT_EQ(0x0807060504U, E5.GetAddress(&offset));
+ EXPECT_EQ(8U, offset);
+
+ // Test 7-byte addresses.
+ offset = 0;
+ EXPECT_EQ(0x0504030201U, E7.GetMaxU64(&offset, 5));
+ EXPECT_EQ(5U, offset);
+ offset = 0;
+ EXPECT_EQ(0x07060504030201U, E7.GetAddress(&offset));
+ EXPECT_EQ(7U, offset);
+}
+
TEST(DataExtractorTest, GetMaxU64) {
uint8_t buffer[] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08};
DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
Index: lldb/source/Utility/DataExtractor.cpp
===================================================================
--- lldb/source/Utility/DataExtractor.cpp
+++ lldb/source/Utility/DataExtractor.cpp
@@ -133,7 +133,7 @@
m_end(const_cast<uint8_t *>(static_cast<const uint8_t *>(data)) + length),
m_byte_order(endian), m_addr_size(addr_size), m_data_sp(),
m_target_byte_size(target_byte_size) {
- assert(addr_size == 4 || addr_size == 8);
+ assert(addr_size >= 1 && addr_size <= 8);
}
// Make a shared pointer reference to the shared data in "data_sp" and set the
@@ -146,7 +146,7 @@
: m_start(nullptr), m_end(nullptr), m_byte_order(endian),
m_addr_size(addr_size), m_data_sp(),
m_target_byte_size(target_byte_size) {
- assert(addr_size == 4 || addr_size == 8);
+ assert(addr_size >= 1 && addr_size <= 8);
SetData(data_sp);
}
@@ -160,7 +160,7 @@
: m_start(nullptr), m_end(nullptr), m_byte_order(data.m_byte_order),
m_addr_size(data.m_addr_size), m_data_sp(),
m_target_byte_size(target_byte_size) {
- assert(m_addr_size == 4 || m_addr_size == 8);
+ assert(m_addr_size >= 1 && m_addr_size <= 8);
if (data.ValidOffset(offset)) {
offset_t bytes_available = data.GetByteSize() - offset;
if (length > bytes_available)
@@ -173,7 +173,7 @@
: m_start(rhs.m_start), m_end(rhs.m_end), m_byte_order(rhs.m_byte_order),
m_addr_size(rhs.m_addr_size), m_data_sp(rhs.m_data_sp),
m_target_byte_size(rhs.m_target_byte_size) {
- assert(m_addr_size == 4 || m_addr_size == 8);
+ assert(m_addr_size >= 1 && m_addr_size <= 8);
}
// Assignment operator
@@ -251,7 +251,7 @@
offset_t data_offset,
offset_t data_length) {
m_addr_size = data.m_addr_size;
- assert(m_addr_size == 4 || m_addr_size == 8);
+ assert(m_addr_size >= 1 && m_addr_size <= 8);
// If "data" contains shared pointer to data, then we can use that
if (data.m_data_sp) {
m_byte_order = data.m_byte_order;
@@ -680,12 +680,12 @@
//
// RETURNS the address that was extracted, or zero on failure.
uint64_t DataExtractor::GetAddress(offset_t *offset_ptr) const {
- assert(m_addr_size == 4 || m_addr_size == 8);
+ assert(m_addr_size >= 1 && m_addr_size <= 8);
return GetMaxU64(offset_ptr, m_addr_size);
}
uint64_t DataExtractor::GetAddress_unchecked(offset_t *offset_ptr) const {
- assert(m_addr_size == 4 || m_addr_size == 8);
+ assert(m_addr_size >= 1 && m_addr_size <= 8);
return GetMaxU64_unchecked(offset_ptr, m_addr_size);
}
@@ -696,7 +696,7 @@
//
// RETURNS the pointer that was extracted, or zero on failure.
uint64_t DataExtractor::GetPointer(offset_t *offset_ptr) const {
- assert(m_addr_size == 4 || m_addr_size == 8);
+ assert(m_addr_size >= 1 && m_addr_size <= 8);
return GetMaxU64(offset_ptr, m_addr_size);
}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
