[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#1838249 <https://reviews.llvm.org/D70840#1838249>, @mstorsjo wrote:

> In D70840#1838186 <https://reviews.llvm.org/D70840#1838186>, @labath wrote:
>
> > 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.


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.

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

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


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

https://reviews.llvm.org/D70840