[Lldb-commits] [PATCH] D99401: Fix .debug_aranges parsing issues introduced with llvm error handling in LLDB

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.cpp:96
   const uint32_t header_size = *offset_ptr - m_offset;
   const uint32_t tuple_size = m_header.addr_size << 1;
   uint32_t first_tuple_offset = 0;
----------------
yinghuitan wrote:
> Unrelated, but I think `2 * m_header.addr_size` is more readable considering compiler would optimize it into bit shifting anyway.
I agree and these kinds of NFC fixes can easily be checked in without review if you want to commit them. 


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.cpp:99
   while (first_tuple_offset < header_size)
     first_tuple_offset += tuple_size;
 
----------------
True, and an NFC commit can be made if you want to take that on.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.cpp:135
+      // entries if they were stripped. We also could watch out for multiple
+      // entries at address zero and remove those as well.
+      if (arangeDescriptor.length > 0)
----------------
The problem is that we might have a .o file that does have a valid function at address zero. Since these descriptors are not sorted it would be inefficient to check for multiple entries with address zero. The problem is that linkers really shouldn't be picking address zero as the tombstone value for "I didn't have a location for this address". An address of UINT64_MAX (-1) would be better and would be easier to see if this is incorrect. The way that GSYM does this is it figures out what the valid section ranges are for executable sections and only accepts address ranges that are in those valid executable ranges. We have some patches to the DWARF parser that tried to go this from Antonio I believe, but I am not sure they ever got resolved and checked in. It is quite a problem though, but should be solved in a separate patch.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.h:61
+  dw_offset_t m_offset;
+  dw_offset_t m_next_offset;
   Header m_header;
----------------
yinghuitan wrote:
> Do you mind add comment to explain this field? It is not very clear that it points to the section offset after this .debug_aranges set. Also, maybe it is more meaningful rename it to `m_set_end_offset`? 
It is the offset of the next DWARFDebugArangeSet. I can add a comment in a follow up patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99401