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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to