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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
