[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.

Looking good, one question left on the test file.



================
Comment at: lldb/source/API/SBValue.cpp:936
+      if (ABISP abi_sp = process_sp->GetABI())
+        return abi_sp->FixCodeAddress(ret_val);
+    return ret_val;
----------------
jasonmolenda wrote:
> 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?
There is FixAnyAddress which is intended to be the least destructive of the 2, but not necessarily the correct one. If we truly have to handle code and data addresses differently I think we'll have a whole ABI/platform/architecture break to worry about on top of that.

Keep it as is. Just flagging it in case you'd had any concerns there, if you've got experience using it and it's fine that's good enough for me.


================
Comment at: lldb/test/API/api/clear-sbvalue-nonadressable-bits/TestClearSBValueNonAddressableBits.py:29
+            self.runCmd ("v v v_p v_invalid_p")
+            self.runCmd ("v &v &v_p &v_invalid_p")
+
----------------
Can you expand the alias here, all the vs are making my head spin.


================
Comment at: lldb/test/API/api/clear-sbvalue-nonadressable-bits/main.c:16
+
+  scratch = (intptr_t) dlsym(RTLD_DEFAULT, "main");
+  scratch |= (3ULL << 60);
----------------
Do you need the `dlsym` here for a specific reason? I'm not familiar with what that does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142792