[Lldb-commits] [PATCH] D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

In D70840#1792498 <https://reviews.llvm.org/D70840#1792498>, @labath wrote:

> In D70840#1791292 <https://reviews.llvm.org/D70840#1791292>, @mstorsjo wrote:
>
> > And irrespectively if the ArchSpec vs Architecture design, can you (either of you) comment on the updated form of the patch?
>
>
> The code still seems somewhat schizophrenic to me. :/ The line tables are fixed up super late,


This bit was based on suggestions by @clayborg here; it could be moved earlier as well, there's not much restrictions on where that one is done.

> but DW_AT_low_pc is adjusted very early. The line table adjustment happens even after sorting, which means the fixup could alter the sort order. It probably wouldn't matter in practice, as everything would just get decremented by one, but it still seems like a bad design. And adjusting the low_pc so early will complicate the move to the llvm dwarf parser.
> 
> I think I'd most prefer some middle ground where the fixup happens after the lowest extraction layers are finished, but before the data hits the "generic" code. It's possible that no such place exists right now, but it might be possible to create something with a bit of refactoring...

The main issue with DW_AT_low_pc/DW_AT_high_pc, is that they're used in many different places - it's not just a straightforward process of "read data from storage into intermediate representations", but there's different accessor methods that all end up going to the original source, fetching the data. `GetDIENamesAndRanges` is one of the methods that reads data and outputs arrays and ranges, and those could be considered to handle later after extracting. `GetAttributeAddressRange` is one that is used in different contexts, where it might be possible to move the fixing up to a higher layer. But then there's cases like `LookupAddress` which seems to fetch the raw data from storage, just to do `if ((lo_pc <= address) && (address < hi_pc))` to see if the info for the desired address is found.

But I guess it could be moved a few steps further out at least; for `LookupAddress` it could be done at the very end after fetching all the addresses, before doing the comparison, for `GetAttributeAddressRange` (which also is used by `LookupAddress`) it could be done right before returning, and with `GetDIENamesAndRanges` it could even be done by the caller.

The problem with the interface of the `DWARFDebugInfoEntry` class is that there's a lot of public methods, that provide access to data both at a high and low level of abstraction. Currently not all of the public methods are called from outside of the object, but the current implementation (where it's done immediately after loading values) was an attempt to safeguard all possible access patterns, even the ones not currently exercised. If we only do it in certain places, we could later end up with new code accessing the lower level methods, bypassing the fixups.


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

https://reviews.llvm.org/D70840