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