[Lldb-commits] [PATCH] D73961: [LLDB] Addresses can be two bytes in size

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.cpp:71
 
-  if (m_header.addr_size != 4 && m_header.addr_size != 8)
     return llvm::make_error<llvm::object::GenericBinaryError>(
----------------
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).


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:812
   bool version_OK = SymbolFileDWARF::SupportedVersion(header.m_version);
-  bool addr_size_OK = (header.m_addr_size == 4) || (header.m_addr_size == 8);
   bool type_offset_OK =
----------------
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.


================
Comment at: lldb/source/Symbol/DWARFCallFrameInfo.cpp:42
   const uint32_t addr_size = DE.GetAddressByteSize();
-  assert(addr_size == 4 || addr_size == 8);
 
----------------
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.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:134
   for (uint32_t I = 0; I < AddrCount; ++I)
-    if (HeaderData.AddrSize == 4)
       Addrs.push_back(Data.getU32(OffsetPtr));
----------------
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").


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73961/new/

https://reviews.llvm.org/D73961