[Lldb-commits] [PATCH] D70532: [lldb] Improve/fix base address selection in location lists

[Greg Clayton via Phabricator via lldb-commits]

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