[Lldb-commits] [PATCH] D142792: Add SBValue::GetValueAsAddress(), strip off ptrauth, TBI, MTE bits on AArch64 systems

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added inline comments.


================
Comment at: lldb/source/API/SBValue.cpp:936
+      if (ABISP abi_sp = process_sp->GetABI())
+        return abi_sp->FixCodeAddress(ret_val);
+    return ret_val;
----------------
DavidSpickett wrote:
> We've talked about this before, and for now I think the result of `FixCode` and `FixData` are the same for debug purposes. In the event that that ever changes, our path is probably to document this as assuming a code address, then adding specific `ValueAsCodeAddress` and `ValueAsDataAddress`, correct?
> 
> (which sounds good to me, just checking we've thought that through)
> 
> You could document that in the docstring but there's a good chance it would cause more confusion than it's worth when at this time, it's not something users have to care about.
Yeah, this is a good point that I punted on a bit.  On Darwin we are only using TCR_EL1.T0SZ to tell us how many bits are used for addressing; there's no distinction between code and data for us.  I guess I should worry about someone defining a `FixCodeAddress` method in an ABI plugin that clears lower bits when instruction alignment is required.  It may be that it's safer to use FixDataAddress if I have to choose between the two without any context.  In our internal implementation in the apple sources, we only had FixAddress which were used for both.  What do you think?


================
Comment at: lldb/source/Core/ValueObject.cpp:1469
 
   case Value::ValueType::HostAddress:
   case Value::ValueType::LoadAddress:
----------------
DavidSpickett wrote:
> What even is a host address, and do we care here?
> ```
>     /// A host address value (for memory in the process that < A is
>     /// using liblldb).
>     HostAddress
> ```
> I'd have to check more but could that be an issue if I was debugging from x86 (where the non-address bits must be 1 or 0) to AArch64?
> 
> Maybe it is just included here so we cover all enum values and in fact, addresses in this path will always be from the debugee.
Excellent point. ValueObjects can point to host memory for sure.  We can have a problem with AArch64 to AArch64 debug session; macs run in a larger address space than iOS devices for instance, so masking off the target iOS bits from a mac side address could make it invalid.


================
Comment at: lldb/source/Core/ValueObject.cpp:3012
     if (pointer_type) {
+      if (Process *process = exe_ctx.GetProcessPtr()) {
+        if (ABISP abi_sp = process->GetABI()) {
----------------
DavidSpickett wrote:
> Probably best on its own change but I wonder if it's time to add Process::Fix<...>Address to dedupe all these conditionals.
Yes, this is a good idea.  I will do this separately.


================
Comment at: lldb/source/DataFormatters/ValueObjectPrinter.cpp:432
+          if (stripped != m_valobj->GetPointerValue()) {
+            m_stream->Printf(" (actual=0x%" PRIx64 ")", stripped);
+          }
----------------
DavidSpickett wrote:
> Is there a way to cover this with a test? I think there is one for the `(actual=` part already, does that cover this part too?
Good point.  And actually, this codepath was never firing because both things it compared to were stripping the non-addressable bits off.  Fixed this method, added to the test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142792