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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

I figured I'd leave some nits on a Friday afternoon.



================
Comment at: lldb/bindings/interface/SBValue.i:117-132
+"     // Return the value as an address.  On failure, LLDB_INVALID_ADDRESS 
+      // will be returned.  On architectures like AArch64, where the top 
+      // (unaddressable) bits can be used for authentication, memory tagging, 
+      // or top byte ignore,  this method will return the value with those 
+      // top bits cleared.  
+      //
+      // GetValueAsUnsigned returns the actual value, with the 
----------------
Nit: no `//` 


================
Comment at: lldb/source/API/SBValue.cpp:932-934
+    if (!success) {
+      return fail_value;
+    }
----------------
Nit: no braces around single life if for consistency with lines 935 and 393.


================
Comment at: lldb/source/API/SBValue.cpp:935-940
+    ProcessSP process_sp = m_opaque_sp->GetProcessSP();
+    if (!process_sp)
+      return ret_val;
+    ABISP abi_sp = process_sp->GetABI();
+    if (abi_sp)
+      return abi_sp->FixCodeAddress(ret_val);
----------------
Nit^3: why not do the same as in `GetStrippedPointerValue` and `CreateValueObjectFromAddress`:

```
    if (ProcessSP process_sp = m_opaque_sp->GetProcessSP())
      if (ABISP abi_sp = process_sp->GetABI())
            return abi_sp->FixCodeAddress(ret_val);
```


================
Comment at: lldb/source/API/SBValue.cpp:938-940
+    ABISP abi_sp = process_sp->GetABI();
+    if (abi_sp)
+      return abi_sp->FixCodeAddress(ret_val);
----------------
Nit^2: in other places where I upstreamed `FixCodeAddress` calls I went with the following LLVM pattern:

```
    if (ABISP abi_sp = process_sp->GetABI())
      return abi_sp->FixCodeAddress(ret_val);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142792