Update DwarfWalker subrange handling #1369
Conversation
This implementation was neither correct nor complete. Moreover, we shouldn't be parsing FORM types. Looking for DW_TAG* is sufficient.
The LONG_MIN/MAX aren't great, but they are propagated to preserve behavior.
This will happen in the callee
hainest
force-pushed
the
update_DwarfWalker_subrange_handling
branch
from
2ad8210
to
ea0e6a9
Compare
dwarf/src/dwarf_subrange.cpp
Outdated
| namespace Dyninst { | ||
| namespace DwarfDyninst { | ||
|
|
||
| dwarf_result<long> dwarf_subrange_upper_bound(Dwarf_Die *die) { |
There was a problem hiding this comment.
int64_t or Dwarf_Word instead of long? OK on linux and everywhere Dyninst runs, but long is required to be only at least 32-bits and would narrow the result if you were on a non-64-bit long system such as win64.
There was a problem hiding this comment.
What about C VLAs and C flexible arrays? The DWARF form type is an exprloc and not a constant integer type. I also see subranges with a type, but without a DW_AT_upper_bound attribute (it also changes an array of array to a single subrange without the DW_AT_upper_bound.
These could be deferred to a new issue: array types need an unknown (and maybe locexpr) upper bound value.
There was a problem hiding this comment.
Switched using Dwarf_Word. VLAs and flexible arrays aren't handled in libdw's dwarf_aggregate_size. Maybe we can just ignore them here, as well?
dwarf/src/dwarf_subrange.cpp
Outdated
| Dwarf_Word count; | ||
| if (dwarf_formudata(&attr, &count) != 0) | ||
| return dwarf_error{}; | ||
| return count; |
There was a problem hiding this comment.
return count + lower_bound - 1?
Works for zero-based array languages, but not 1-based arrays for languages or those that support a true range for the index values. Perhaps for efficiency there should only a function to return both lower and upper bounds as a pair so lower bound doesn't need to be computed twice if DW_AT_count is used.
There was a problem hiding this comment.
Agreed. I refactored this with the added assumption of zero-based if there is DW_AT_count but no DW_AT_lower_bound or dwarf_srclang_lower_bound.
This was accidentally included.
C++ doesn't guarantee it is convertible to `long`.
The calculation for a range's upper bound when DW_AT_count is used requires knowing the lower bound (if given), so these needed to be merged.
|
Updates are testing fine. |
This effectively rewrites the handling of subranges and arrays in DwarfWalker by
parseMultidimensionalArray)