[LLDB] Addresses can be two bytes in size

[aykevl]

Summary

Addresses are usually two bytes in size on AVR, so make sure LLDB deals with that.

---

I'm not sure I caught all cases. In particular, lldb/include/lldb/DataFormatters/FormattersHelpers.h might be related but I've left it alone as it didn't seem to be necessary.

Honestly I think LLDB would become a lot more forgiving to uncommon architectures when these asserts are removed and avoids assumptions based on the address size. When the address size is relevant (for example, to read a value from memory), it should have an exhaustive test with a default case that does an assert (for easy debugging). For example:

switch (addr_size) {
case 4:
    // do one thing
case 8:
    // do something else
default:
    assert(false && "unknown addr_size");
}

This way, it's easy to find the places that make these assumptions just by following the asserts.

I'm not sure how to add a test for this. I can test it locally by connecting to a remote debugger (simavr). However, I don't know how to do something like that in the LLDB testing framework. Additionally, for that to work I only needed the change in DumpDataExtractor.cpp so I'm not sure how to exhaustively test this.

[aykevl]

See D73969.

[labath]

You should be able to test most (if not all) of this stuff without an actual process. lldb can display global variables, set breakpoints, resolve addresses, etc. without being connected to a debug stub.

How do you normally generate the binaries with these kinds of addresses? Can they be produced with clang? Can they be produced with llvm-mc? Linked with lld ?

[aykevl]

How do you normally generate the binaries with these kinds of addresses? Can they be produced with clang? Can they be produced with llvm-mc? Linked with lld ?

With avr-gcc. I think it's easiest to just generate a minimal binary using yaml2obj.

For the rest I'm focusing on D73969 now, this patch should probably be abandoned.

[labath]

In D73961#1860270, @aykevl wrote:

How do you normally generate the binaries with these kinds of addresses? Can they be produced with clang? Can they be produced with llvm-mc? Linked with lld ?

With avr-gcc. I think it's easiest to just generate a minimal binary using yaml2obj.

For the rest I'm focusing on D73969 now, this patch should probably be abandoned.

yaml2obj can certainly be useful, the yaml files are not that readable when you need to create more complicated inputs (e.g. with DWARF). Writing assembly could be more useful there.

But anyway, we can evaluate that on a case-by-case basis.

[aykevl]

Closing this one. Maybe there is something useful in this patch but if there is, I'll submit that in a standalone (more focused) patch. The majority of it has been merged in D73969.

[labath]

Yes, standalone patches are the way to go. To help you, I've tried to annotate the various assertions, what kind of problems they could cause, and possible testing strategies.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.cpp

71

I don't believe this will cause any failures, but it will stop lldb from using the debug_aranges section (and fall back to other, potentially slower, alternatives).

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

812

This will definitely mean you won't be able to parse any (DWARF) debug info. For a test inspiration, you could look at the existing .s files in test/Shell/SymbolFile/DWARF.

lldb/source/Symbol/DWARFCallFrameInfo.cpp

42

This will probably cause some problems when unwinding via eh/debug_frame. Testing will be somewhat tricky, as I don't think we'll parse this without a running process. I'd leave this one for the end.

llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

134

This one is also pretty critical for DWARF debug info. You could probably test it together with the DWARFUnit thingy. To trigger, it should be enough to try to display a global variable ("target variable my_global").

lldb/source/Core/DumpDataExtractor.cpp

uint32_t item_bit_size,   // If zero, this is not a bitfield value, if
                               // non-zero, the value is a bitfield
     uint32_t item_bit_offset, // If "item_bit_size" is non-zero, this is the
                               // shift amount to apply to a bitfield
     ExecutionContextScope *exe_scope) {
   if (s == nullptr)
     return start_offset;
 
   if (item_format == eFormatPointer) {
-    if (item_byte_size != 4 && item_byte_size != 8)
+    if (item_byte_size != 2 && item_byte_size != 4 && item_byte_size != 8)
       item_byte_size = s->GetAddressByteSize();
   }
 
   offset_t offset = start_offset;
 
   if (item_format == eFormatInstruction) {
     TargetSP target_sp;
     if (exe_scope)

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.cpp

llvm::Error DWARFDebugArangeSet::extract(const DWARFDataExtractor &data,
   // 1 - the version looks good
   // 2 - the address byte size looks plausible
   // 3 - the length seems to make sense
   // size looks plausible
   if (m_header.version < 2 || m_header.version > 5)
     return llvm::make_error<llvm::object::GenericBinaryError>(
         "Invalid arange header version");
 
-  if (m_header.addr_size != 4 && m_header.addr_size != 8)
+  if (m_header.addr_size != 2 && m_header.addr_size != 4 && m_header.addr_size != 8)
     return llvm::make_error<llvm::object::GenericBinaryError>(
         "Invalid arange header address size");
 
   if (m_header.length == 0)
     return llvm::make_error<llvm::object::GenericBinaryError>(
         "Invalid arange header length");
 
   if (!data.ValidOffset(m_offset + sizeof(m_header.length) + m_header.length -

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

DWARFUnitHeader::extract(const DWARFDataExtractor &data, DIERef::Section section,
 
   if (header.IsTypeUnit()) {
     header.m_type_hash = data.GetU64(offset_ptr);
     header.m_type_offset = data.GetDWARFOffset(offset_ptr);
   }
 
   bool length_OK = data.ValidOffset(header.GetNextUnitOffset() - 1);
   bool version_OK = SymbolFileDWARF::SupportedVersion(header.m_version);
-  bool addr_size_OK = (header.m_addr_size == 4) || (header.m_addr_size == 8);
+  bool addr_size_OK = (header.m_addr_size == 2) || (header.m_addr_size == 4) || (header.m_addr_size == 8);
   bool type_offset_OK =
       !header.IsTypeUnit() || (header.m_type_offset <= header.GetLength());
 
   if (!length_OK)
     return llvm::make_error<llvm::object::GenericBinaryError>(
         "Invalid unit length");
   if (!version_OK)
     return llvm::make_error<llvm::object::GenericBinaryError>(

lldb/source/Symbol/DWARFCallFrameInfo.cpp

GetGNUEHPointer(const DataExtractor &DE, offset_t *offset_ptr,
                 addr_t data_addr) //, BSDRelocs *data_relocs) const
 {
   if (eh_ptr_enc == DW_EH_PE_omit)
     return ULLONG_MAX; // Value isn't in the buffer...
 
   uint64_t baseAddress = 0;
   uint64_t addressValue = 0;
   const uint32_t addr_size = DE.GetAddressByteSize();
-  assert(addr_size == 4 || addr_size == 8);
+  assert(addr_size == 2 || addr_size == 4 || addr_size == 8);
 
   bool signExtendValue = false;
   // Decode the base part or adjust our offset
   switch (eh_ptr_enc & 0x70) {
   case DW_EH_PE_pcrel:
     signExtendValue = true;
     baseAddress = *offset_ptr;
     if (pc_rel_addr != LLDB_INVALID_ADDRESS)

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 == 2 || addr_size == 4 || 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 == 2 || addr_size == 4 || 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 == 2 || m_addr_size == 4 || 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 == 2 || m_addr_size == 4 || 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 == 2 || m_addr_size == 4 || 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 == 2 || m_addr_size == 4 || 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 == 2 || m_addr_size == 4 || m_addr_size == 8);
   return GetMaxU64_unchecked(offset_ptr, m_addr_size);
 }
 
 // Extract a single pointer from the data and update the offset pointed to by
 // "offset_ptr". The size of the extracted pointer comes from the
 // "this->m_addr_size" member variable and should be set correctly prior to
 // extracting any pointer values.
 //
 // 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 == 2 || m_addr_size == 4 || m_addr_size == 8);
   return GetMaxU64(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()) {

llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

return createStringError(errc::invalid_argument,
                        ".debug_addr table at offset 0x%" PRIx64
                        " contains data of size %" PRIu32
                        " which is not a multiple of addr size %" PRIu8,
                        HeaderOffset, DataSize, HeaderData.AddrSize);
   }
   Data.setAddressSize(HeaderData.AddrSize);
   uint32_t AddrCount = DataSize / HeaderData.AddrSize;
   for (uint32_t I = 0; I < AddrCount; ++I)
-    if (HeaderData.AddrSize == 4)
+    if (HeaderData.AddrSize == 2)
+      Addrs.push_back(Data.getU16(OffsetPtr));
+    else if (HeaderData.AddrSize == 4)
       Addrs.push_back(Data.getU32(OffsetPtr));
     else
       Addrs.push_back(Data.getU64(OffsetPtr));
   return Error::success();
 }
 
 void DWARFDebugAddrTable::dump(raw_ostream &OS, DIDumpOptions DumpOpts) const {
   if (DumpOpts.Verbose)