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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
