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

[jeffrey tan via Phabricator via lldb-commits]

yinghuitan 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;
----------------
Unrelated, but I think `2 * m_header.addr_size` is more readable considering compiler would optimize it into bit shifting anyway.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.cpp:98-99
   uint32_t first_tuple_offset = 0;
   while (first_tuple_offset < header_size)
     first_tuple_offset += tuple_size;
 
----------------
Unrelated to this diff, but I find this simple round up can be shorten to:
```
first_tuple_offset = ((header_size + tuple_size - 1) / tuple_size) * tuple_size;
```


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.cpp:134-135
+      // Some linkers will zero out the length field for some .debug_aranges
+      // 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)
----------------
Maybe add a TODO for "We also could watch out for multiple entries at address zero and remove those as well" since we did not do it here?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.h:61
+  dw_offset_t m_offset;
+  dw_offset_t m_next_offset;
   Header m_header;
----------------
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`? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99401