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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> Script authors may want access to both the actual uint64_t value, and the address that will be accessed, in an SBValue, so I added a new method in addition to GetValueAsUnsigned to provide this.

In my rough cut of API stuff for memory tagging I had:

  frame  = process.GetThreadAtIndex(0).GetFrameAtIndex(0)
  mte_buf = frame.FindVariable("mte_buf").GetValueAsUnsigned()
  
  # TODO: later we need better access to ABI
  err = SBError()
  mte_buf = process.FixDataAddress(mte_buf, err)

What you have here is a much better way of doing it.

Memory tagging is a use case for having both. When you get the tags back you want to check if the tag for the memory matches the one in the unmodified pointer.

> I have the attached test case set to run on any AArch64 system; on Darwin we do the same pointer stripping on any process regardless if it is using pointer auth (that is, for both "arm64" and "arm64e"). On Linux, a non-ptrauth process may not have an address mask and this test may fail because the bits > I mask into the top nibble in the test program are not removed by GetValueAsAddress(). I'm not sure exactly, but I can remove this test from running on Linux, or add a check for isAArch64PAuth or something. This program never actually dereferences this pointer value with bits in the high nibble set, it's > only set up for lldb to manipulate.

I ran the test on AArch64 Linux without PAuth and it passes. AArch64 Linux always has TBI enabled for userspace (https://www.kernel.org/doc/html/latest/arm64/tagged-address-abi.html) (well, on by default but I am yet to see anyone disable it).

So lldb sees:

  (lldb) process status -v
  <...>
  Addressable code address mask: 0xff00000000000000
  Addressable data address mask: 0xff00000000000000
  Number of bits used in addressing (code): 56

(and if you have PAuth on that mask just has more bits set)

So it's fine to run this on Linux.

BSDs and Windows on Arm will not have an address mask. I would explicitly list Linux and Mac OS for the test.



================
Comment at: lldb/bindings/interface/SBValue.i:124
+      GetValueAsUnsigned returns the actual value, with the
+      authentication/TBI/MTE bits.
+
----------------
Just for clarity, use the full names again here.

"with the authentication/top byte ignore/memory tagging bits"


================
Comment at: lldb/bindings/interface/SBValue.i:126-127
+
+      Calling this on a random value which is not a pointer is an
+      incorrect.  Call GetType().IsPointerType() if in doubt.
+
----------------
"is incorrect", drop the "an"


================
Comment at: lldb/source/API/SBValue.cpp:936
+      if (ABISP abi_sp = process_sp->GetABI())
+        return abi_sp->FixCodeAddress(ret_val);
+    return ret_val;
----------------
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.


================
Comment at: lldb/source/Core/ValueObject.cpp:1469
 
   case Value::ValueType::HostAddress:
   case Value::ValueType::LoadAddress:
----------------
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.


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


================
Comment at: lldb/source/DataFormatters/ValueObjectPrinter.cpp:432
+          if (stripped != m_valobj->GetPointerValue()) {
+            m_stream->Printf(" (actual=0x%" PRIx64 ")", stripped);
+          }
----------------
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?


================
Comment at: lldb/test/API/api/clear-sbvalue-nonadressable-bits/TestClearSBValueNonAddressableBits.py:16
+    @skipIf(archs=no_match(['aarch64', 'arm64', 'arm64e']))
+
+    def test(self):
----------------
I would add skipif operating system not Mac OS or Linux.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142792