[Lldb-commits] [PATCH] D155905: lldb RFC: Exposing set/get address masks, Fix*Address methods in SBProcess

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.


================
Comment at: lldb/include/lldb/API/SBProcess.h:433
+
+  /// Clear the non-addressable bits of an \a addr value and return a
+  /// virtual address in memory.
----------------
"Clear bits from an addr value that are not used for addressing" is clearer to me.

Addressable makes me think I can craft an address to point to that bit, and I've been saying "non-address" but that term is also inside baseball, so something more wordy I think is better.


================
Comment at: lldb/include/lldb/API/SBProcess.h:445
+  lldb::addr_t FixDataAddress(lldb::addr_t addr);
+  lldb::addr_t FixAnyAddress(lldb::addr_t addr);
+
----------------
jasonmolenda wrote:
> clayborg wrote:
> > What does this function do? Does it try to auto detect code/data low/hi on its own assuming that specific regions are easily identifiable?
> We have the same method in Process.  Linux has separate masks for code & data addresses, so we have FixCode and FixData methods, but sometimes you just have an addr_t and you don't know the context (e.g. a register value that you're testing to see if it points to a symbol so you can print that as additional info to the user), and so in that case we have a Process::FixAddress.
> 
> There is one armv7 abi that clears the 0th bit on FixCode address, and I could see other ABIs doing similar things for the fixed nature of code addresses.  But Data addresses are the most general, having to point to any addressable byte.
> 
> Our Process/SBProcess say "FixCode if you know it's code, FixData if you know it's data.  And if you don't know, FixData".  But I think having a "FixAddress" method, making it clear that you don't know the origin of the value, makes the intention a little clearer.  Even if it's just calling FixData inside Process.
FixAnyAddress is also a marker for the, hopefully far from now, time, that lldb has to track address provenance. That's not an API concern though.

Should these functions also be one function that takes the enum value?

It's `FixAddress(addr, lldb.eMaskTypeData` instead of `FixDataAddress(addr)`, but it keeps the theme consistent between all the functions.


================
Comment at: lldb/include/lldb/API/SBProcess.h:401
 
+  /// Get the current address mask that may be applied to addresses
+  /// before reading from memory.  The bits which are not relevant/valid
----------------
jasonmolenda wrote:
> DavidSpickett wrote:
> > may -> should?
> > 
> > Though for API uses that only run on simple systems, calling this would just be unnecessary overhead.
> Probably the right call.  The masking is implemented in the ABI so there are systems that don't have this concept and have no mask function, which is what I was thinking of with "maybe".  
> 
> It makes me wonder if doing it in the ABI is correct; if we have an address mask for the system, is it a part of the ABI?  On an ARMv8.3 ptrauth using process, the ABI defines what things are signed and need to be cleared before dereferencing (where we need to call FixAddress from within lldb), but we solve this by calling FixAddress at all sites that any ABI might use; ultimately they all need to resolve to an addressable address before reading, so if one ABI signs the C++ vtable pointer and one does not, having both call FixAddress on that is fine.
> 
> Of course I'm coming at this with an AArch64 perspective.  On AArch64 we need to look at b55 to decide if the bits that are masked off are set to 0 or 1  (bits 56-63 possibly being TBI non-address bits), which is not a generic behavior that would apply on other processors that might need to apply an address mask.  So maybe it's an architecture specific method instead of an ABI method, I should say.
I don't think it being in the ABI plugin is particularly deliberate, even if it is correct.

The ABI states something like the top byte is reserved for the OS and language runtimes, lldb is dealing with a consequence of that choice. So you could argue the ABI plugin should say "yes this should be fixed" and the fixing should happen elsewhere.

But right now those two things are so close together I don't think it matters.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155905