clayborg added a comment. In D70532#1758962 <https://reviews.llvm.org/D70532#1758962>, @labath wrote:
> In D70532#1758907 <https://reviews.llvm.org/D70532#1758907>, @clayborg wrote: > > > So I am worried by seeing any mention of "load address" and "load bias" in > > this patch. All information that is extracted from the DWARF should be > > solely in terms if "file addresses" IMHO, > > > This is still true. The information for which goes into the construction of > the DWARFExpression is based on information from the object file alone -- > this is the CU base (file) address, and function base (file) address. > Essentially, the only difference here is that previously we were passing the > difference between these two values, whereas this patch passes the two values > separately. Gotcha. > > > The "load" address goes in only when the expression is evaluated (and so it > can be different for each invocation, etc.). This part is not changed at all, > though I have renamed the value to better reflect how it is now used in the > patch. > > > Let's say we have a CU whose base address is 0x1000, and a function which > starts at 0x1100, and we also have a location list like: (0x142, 0x147) => > DW_OP_whatever. Let's say that the module is loaded in memory such that the > function now starts at 0x8100. > What happened previously that we would create the DWARFExpression with the > "slide" of 0x100. Then, when we wanted to get an actual location we'd pass in > 0x8100 as the "load address", and the code would compute > (0x8100 - 0x100 + 0x142, 0x8100 - 0x100 + 0x147) = (0x8142, 0x8147) to get > the actual location list range. > This worked fine if you didn't have base address selection entries in the > location list. Once you have those, you have a problem -- you know that you > need to change the base address somehow, but it's not possible to compute > how. If we pass in the "file" address of the function start (0x1100), we can > compute the load bias (0x8100 - 0x1100 = 0x7000), and add that to any address > coming out of the location list. Thanks for explaining. >> For mach-o DWARF in .o files, the "file address" will be a file address from >> the .o file (which is fine), but it will need to be linked into an >> executable file address before being handed out. We fix up addresses like >> this for any DW_OP_addr opcodes in location expressions. So typically we >> would have a virtual SymbolFileDWARF method that SymbolFileDWARFDebugMap >> would override and this would use the default >> SymbolFileDWARF::GetLocationList(...) function, and then link up the >> location list or DWARFExpression prior to handing it out. Is there something >> I am not understanding here? > > The process you're describing is indeed used elsewhere (line tables for > instance). It's not possible to use it here, because the location lists are > parsed very late -- basically until you actually start to evaluate the > expression, the location list is just a blob of bytes, so the relocation > needs to happen at a later stage. I'm not sure why something similar wasn't > done here too (I suspect it has something to do with DWARFExpression living > separately from the rest of the DWARF plugin code), but changing that here > seems complicated (particularly as I don't know all the details of mach-o > linker stuff). Sounds good. ================ Comment at: lldb/include/lldb/Expression/DWARFExpression.h:88-89 /// - /// \param[in] process - /// The process to use when resolving the load address + /// \param[in] load_function_start + /// The actual address of the function containing this location list. /// ---------------- Is this a "file address"? Maybe clarify here in the comment, or rename variable appropriately? ================ Comment at: lldb/include/lldb/Expression/DWARFExpression.h:141-143 + /// \param[in] base_address + /// The base address to use for interpreting relative location list + /// entries. ---------------- Maybe change name to "cu_file_addr"? ================ Comment at: lldb/include/lldb/Expression/DWARFExpression.h:144 + /// entries. + /// \param[in] file_function_start + /// The file address of the function containing this location list. This ---------------- Maybe change name to "func_file_addr"? ================ Comment at: lldb/include/lldb/Expression/DWARFExpression.h:148 + /// conjuction with the load_function_start arguments). + void SetLocationListSlide(lldb::addr_t base_address, + lldb::addr_t file_function_start); ---------------- Rename from "SetLocationListSlide" to "SetLocationListAddresses"? Slide doesn't seem relevant anymore ================ Comment at: lldb/include/lldb/Expression/DWARFExpression.h:164 bool Evaluate(ExecutionContextScope *exe_scope, - lldb::addr_t loclist_base_load_addr, + lldb::addr_t load_function_start, const Value *initial_value_ptr, const Value *object_address_ptr, ---------------- Maybe change name to "func_file_addr"? ================ Comment at: lldb/include/lldb/Expression/DWARFExpression.h:227 bool DumpLocationForAddress(Stream *s, lldb::DescriptionLevel level, - lldb::addr_t loclist_base_load_addr, + lldb::addr_t load_function_start, lldb::addr_t address, ABI *abi); ---------------- Maybe change name to "func_file_addr"? ================ Comment at: lldb/include/lldb/Expression/DWARFExpression.h:261 - bool GetLocation(lldb::addr_t base_addr, lldb::addr_t pc, + bool GetLocation(lldb::addr_t load_function_start, lldb::addr_t pc, lldb::offset_t &offset, lldb::offset_t &len); ---------------- Maybe change name to "func_file_addr"? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70532/new/ https://reviews.llvm.org/D70532 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
