[LLDB] Let DataExtractor deal with two-byte addresses

[aykevl]

Summary

AVR usually uses two byte addresses. By making DataExtractor deal with this, it is possible to load AVR binaries that don't have debug info associated with them.

---

This is a more focused version of D73961. With this, I can load an avr-gcc compiled binary and connect to a gdb-remote as long as it doesn't include any instructions that cannot yet be disassembled (see D73911 and D73958).

[aykevl]

Added a unit test.

I think it would also be a good idea to test this by loading an AVR binary and connecting to a debugger (thus triggering this code), but I don't know how to add such a test. Suggestions would be appreciated.

[labath]

I think this is a much better approach than the other do-in-all-together patch -- we should take this one step at a time.

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 agree that more tests would be useful, but I would not consider that as a requirement for this patch. If you want, you can submit extra tests in separate patches.

[shafik]

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?

[aykevl]

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. Maybe this is worth deduplicating eventually.

[labath]

In D73969#1860253, @aykevl wrote:

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.

Awesome, thanks.

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?

I agree only one of those two places should be enough. My idea was to make the constructors delegate to one another (if necessary by creating a private uber-constructor that everybody can delegate to). Then we could have only one assert.

But if that's hard for some reason, your proposal seems fine too.

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. Maybe this is worth deduplicating eventually.

Indeed, I think I'll do that when I find a bit of time.

[aykevl]

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?

I agree only one of those two places should be enough. My idea was to make the constructors delegate to one another (if necessary by creating a private uber-constructor that everybody can delegate to). Then we could have only one assert.

Should I do that in the same patch, or in a different one? Before or after this patch? It seems like doing the constructor merging in this patch would make it more complicated than necessary.

[labath]

In D73969#1861494, @aykevl wrote:

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?

I agree only one of those two places should be enough. My idea was to make the constructors delegate to one another (if necessary by creating a private uber-constructor that everybody can delegate to). Then we could have only one assert.

Should I do that in the same patch, or in a different one? Before or after this patch? It seems like doing the constructor merging in this patch would make it more complicated than necessary.

My hope is that this will be a relatively simple thing, in which case it would be ok to put it into this patch. But I am fine with doing it separately as well...

[dylanmckay]

This looks good to me from an AVR point of view. I don't have very much experience with LLDB however.

[aykevl]

Rebase on master (after GetPointer was removed).

[aykevl]

In D73969#1860696, @labath wrote:

I agree only one of those two places should be enough. My idea was to make the constructors delegate to one another (if necessary by creating a private uber-constructor that everybody can delegate to). Then we could have only one assert.

But if that's hard for some reason, your proposal seems fine too.

Honestly I don't feel very comfortable changing the constructors. I'm not very experienced with C++ and I'm afraid I mess something up. I could remove them from all constructors, reducing the number of asserts from 7 to 3?

[labath]

I think this is fine. There's no need to hold this up over the assert duplication issue, as this does not make the situation any worse.

Thanks for the patch.

[aykevl]

Thanks for the review!

lldb/source/Utility/DataExtractor.cpp

 // data must stay around as long as this object is valid.
 DataExtractor::DataExtractor(const void *data, offset_t length,
                              ByteOrder endian, uint32_t addr_size,
                              uint32_t target_byte_size /*=1*/)
     : m_start(const_cast<uint8_t *>(static_cast<const uint8_t *>(data))),
       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
 // endian swapping setting to "swap", and the address size to "addr_size". The
 // shared data reference will ensure the data lives as long as any
 // DataExtractor objects exist that have a reference to this data.
 DataExtractor::DataExtractor(const DataBufferSP &data_sp, ByteOrder endian,
                              uint32_t addr_size,
                              uint32_t target_byte_size /*=1*/)
     : 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);
 }
 
 // Initialize this object with a subset of the data bytes in "data". If "data"
 // contains shared data, then a reference to this shared data will added and
 // the shared data will stay around as long as any object contains a reference
 // to that data. The endian swap and address size settings are copied from
 // "data".
 DataExtractor::DataExtractor(const DataExtractor &data, offset_t offset,
                              offset_t length, uint32_t target_byte_size /*=1*/)
     : 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)
       length = bytes_available;
     SetData(data, offset, length);
   }
 }
 
 DataExtractor::DataExtractor(const DataExtractor &rhs)
     : 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
 const DataExtractor &DataExtractor::operator=(const DataExtractor &rhs) {
   if (this != &rhs) {
     m_start = rhs.m_start;
     m_end = rhs.m_end;
     m_byte_order = rhs.m_byte_order;

 // pointer to that data will be made in this object. If "data" doesn't contain
 // a shared pointer to data, then the bytes referred to in "data" will need to
 // exist at least as long as this object refers to those bytes. The address
 // size and endian swap settings are copied from the current values in "data".
 lldb::offset_t DataExtractor::SetData(const DataExtractor &data,
                                       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;
     return SetData(data.m_data_sp, data.GetSharedDataOffset() + data_offset,
                    data_length);
   }
 
   // We have a DataExtractor object that just has a pointer to bytes

 
 // Extract a single address from the data and update the offset pointed to by
 // "offset_ptr". The size of the extracted address comes from the
 // "this->m_addr_size" member variable and should be set correctly prior to
 // extracting any address values.
 //
 // 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);
 }
 
 size_t DataExtractor::ExtractBytes(offset_t offset, offset_t length,
                                    ByteOrder dst_byte_order, void *dst) const {
   const uint8_t *src = PeekData(offset, length);
   if (src) {
     if (dst_byte_order != GetByteOrder()) {

lldb/unittests/Utility/DataExtractorTest.cpp

TEST(DataExtractorTest, GetCStrAtNullOffset) {
   uint8_t buffer[] = {'f', 'o', 'o', '\0'};
   DataExtractor E(buffer, sizeof buffer, lldb::eByteOrderLittle, 4);
 
   lldb::offset_t offset = 0;
   EXPECT_STREQ("foo", E.GetCStr(&offset));
   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,
                    sizeof(void *));
   DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig, sizeof(void *));
 
   lldb::offset_t offset;