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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D70840#1844318 <https://reviews.llvm.org/D70840#1844318>, @mstorsjo wrote:

> In D70840#1844279 <https://reviews.llvm.org/D70840#1844279>, @labath wrote:
>
> > This is something that probably should be discussed with the llvm dwarf owners too (though most of them are on this review already).
> >
> > However, if we assume that we can agree that this logic should be implemented at a fairly low level in the dwarf parsing code (so that e.g., when we switch to the llvm parser, it will do this thing for us),
>
>
> I guess the main question here, as follows from my comment above, is who/how is the fixup controlled in the llvm side, where there's (to my knowledge) no `GetOpcodeLoadAddress` that can abstract away the fixup? IIRC the dwarf code itself is happily unaware of what architecture it is describing.


Yes, that is the trickiest part. Dwarf is architecture-agnostic for the most part (which makes this behavior of the windows linker very unfortunate), but the llvm DWARFContext already knows the architecture of the file it is describing (DWARFContext::getArch). The fixup could key off of that, but this will need careful consideration.

> 
> 
>> then this patch still seems quite "schizophrenic" to me, because the line table and location list fixups happen pretty high up. The location list case is particularly interesting here because we use a high level api (`llvm::DWARFLocationExpression`) to retrieve the location list, which may be reasonably expected to return "fixed" addresses, and so this fix should happen in llvm.
> 
> FWIW, as @clayborg said that `DWARFExpression` owns a Module in these cases, and thus should have access to an ArchSpec, I could just slip the fixup into `DWARFExpression::SetLocationListAddresses`. The reason I was messing around there was primarily to avoid sprinkling fixups in many codepaths in `DWARFDebugInfoEntry::GetDIENamesAndRanges` that call this method.

Here, I wasn't referring to `SetLocationListAddresses`, but rather to the addresses you get from the location list section (DWARFExpression::GetLocationExpression). I would expect you need to fix those do (but you don't do it in this patch). As for `SetLocationListAddresses`, I would still consider that high level, as the addresses pass through several high-level functions before landing here. If we go with the low-level approach, I'd expect this to happen somewhere near `DWARFUnit::GetBaseAddress` and `DWARFDIE::getLowPC` (imaginary function inspired by `llvm::DWARFDie::getLowAndHighPC`)

> 
> 
>> (The line tables also use llvm code for parsing, but we access them using  a very low-level api, so it's likely this check would have to done on lldb side even if llvm provided such a functionality -- but this could happen in SymbolFileDWARF::ParseLineTable instead of in the LineTable class).
> 
> Sure, I'll look into moving the fixups there.

I wouldn't bother with that too much until we figure out the overall strategy here.

>> If you want, I can send the initial email to start the discussion, but I can't commit to writing any of the patches. :(
> 
> If you can do that, that would be very much appreciated!

OK, I'll try to get the ball rolling there.


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

https://reviews.llvm.org/D70840