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

Tue Jan 28 05:09:22 PST 2020

mstorsjo added a comment.

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

> Right, your answer implicitly assumes (and I do agree with you) that the answer is that `getSubroutineForAddress` should return the function that address is in. That is not unreasonable -- I deliberately chose the most high-level api I could find, to make arguing that case easy. But that case should be argued nonetheless. For example, a counterargument to that might be that one should only use valid PC register values as inputs to this function (so an even value would not be a valid input here, because the function is thumb), and in that case, the there would be no fixups needed. This position could be further strengthened by the fact that the llvm dwarf parser is also used by tools (e.g., llvm-dwarfdump) which want a fairly exact view of the data in the file, without too much smartness, so there may need to be some kind of an api which would provide the "raw" value.

I guess the selection of what kind of fixups to apply could be controlled via whatever object controls the fixup. In these patches, it's lldb's ArchSpec - the low level dwarf routines doesn't have any ArchSpec but uses the one given to them to fix up the addresses.

> 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.

> 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.

> (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.

> So, at the end of the day, I guess what I am saying is that we should send a small RFC to the llvm debug info folks to clarify what should the behavior of the llvm classes be for this kind of dwarf. If the conclusion is that this should be implemented inside these classes (and not by their users), then we should implement it there, and then have the lldb code mirror that. This is the approach we've been trying to do for a while now when modifying or implementing new dwarf features in lldb, but this is probably the first case of wanting to implement a feature in lldb where there is no existing llvm counterpart. (In contrast the DWZ feature is also blocked in part due to concerns about increasing llvm divergence, but there I'm pretty sure the answer is that DWZ should not be implemented in llvm, and the lldb parts should be written in a way that does not require low-level dwarf changes.)
> 
> 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!

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

https://reviews.llvm.org/D70840