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

Fri Jan 24 02:04:52 PST 2020

mstorsjo added a comment.

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

> Yes, I was keeping this problem in mind when I was working on that patch. :) I believe more work could be done to reduce the number of places that parse DW_AT_low/high_pc, but I haven't gotten around to that yet..

Thanks :-)

Ok - this patch at least shows which codepaths I think are touching it at the moment. In addition to DW_AT_low/high_pc, there's also the case of DW_AT_ranges; I think all relevant paths that access that are taken care of here.

One downside to moving the fixups further out from the parsing, is that it easily becomes more brittle. If a new codepath accesses some getter without doing the fixup afterwards, we'd end up with the same hard-to-diagnose brokenness again.

> The thing that I am not sure we have fully explored is whether there is any need for this `&~1` business in the llvm dwarf code. For instance, what should `llvm::DWARFUnit::getSubroutineForAddress` return if you pass it an address that is equal to the actual memory address of the start of the thumb function, but the relevant DW_AT_low_pc contains `address|1`? Answering that might give us an indication on what is the layer at which this fixup should be applied.

>From a quick check at the code there, for that particular case, either it'd be done in `llvm::DWARFUnit::updateAddressDieMap` or in `llvm::DWARFDie::getAddressRanges`. Since all levels of the API is public, the fix does become safer the closer to parsing it is, but one also generally has less context to work with in those cases.

I think the most narrow interface to apply the fix in would be `DWARFDebugInfoEntry::GetAttributeAddressRanges` (plus `DWARFDebugInfoEntry::GetAttributeAddressRange` which is used a few places internally in `DWARFDebugInfoEntry`) and `DWARFDebugInfoEntry::GetDIENamesAndRanges` (plus line tables); if only applied there, I think it would cover all the cases I'm fixing up at the moment...

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

https://reviews.llvm.org/D70840