[Lldb-commits] [PATCH] D96460: [LLDB] Arm64/Linux Add MTE and Pointer Authentication registers

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:79
 
+  m_mte_ctrl_reg = 0;
+
----------------
Should m_pac_mask also be initialised or is it already?


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:120
+  uint32_t result = 0;
+  if (*auxv_at_hwcap & HWCAP_PACA)
+    result |= ARM64_REGS_CONFIG_PAUTH;
----------------
I think this should be more defensive:
```
if (auxv_at_hwcap && (*auxv_at_hwcap & HWCAP_PACA))
```

Otherwise you'll deference an invalid optional, which might work if its storage happens to be zeroed but it's not intended use.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:126
+
+  if (*auxv_at_hwcap2 & HWCAP2_MTE)
+    result |= ARM64_REGS_CONFIG_MTE;
----------------
Same here, auxv_at_hwcap2 could be llvm::None.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:531
 
+bool NativeRegisterContextLinux_arm64::IsPAuth(unsigned reg) const {
+  if (GetRegisterInfo().IsPAuthReg(reg))
----------------
Can you simplify these?
```
return GetRegisterInfo().IsPAuthReg(reg);
```

(the others could do that too, but obviously we're ignoring those ones here)


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:1100
   m_sve_header_is_valid = false;
+  m_pac_mask_is_valid = false;
 
----------------
I think `m_mte_ctrl_is_valid` is missing here.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:1198
+
+  if (m_mte_ctrl_is_valid)
+    return error;
----------------
Just to confirm I understand this logic.

If `m_mte_ctrl_is_valid` would mean that our cached value of the register is valid. So if something tries to read a new copy of it we fail because they should have used the cached version?

Then if `m_mte_ctrl_is_valid` is false, our cache is out of date so we do the read.

Seems odd to error on asking for a read of a value that is cached but then again I don't know the surrounding code too well. If that pattern is already established no point disturbing it now.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:1216
+
+  error = ReadMTEControl();
+  if (error.Fail())
----------------
This is a sanity check that we didn't ask to write this register on a system that doesn't have it?


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h:182
+
+  void *GetMTEControl() { return &m_pac_mask; }
+
----------------
This looks like a mistake.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:320
+  uint32_t pa_regnum = m_dynamic_reg_infos.size();
+  for (uint32_t i = 0; i < 2; i++) {
+    pauth_regnum_collection.push_back(pa_regnum + i);
----------------
Can you get this 2 from some list somewhere in this class? E.g. PauthRegs.Size(). Or add a comment if not.

g_register_infos_pauth perhaps?


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:324
+    m_dynamic_reg_infos[pa_regnum + i].byte_offset =
+        m_dynamic_reg_infos[pa_regnum + i - 1].byte_offset +
+        m_dynamic_reg_infos[pa_regnum + i - 1].byte_size;
----------------
Is it possible at this point that `m_dynamic_reg_infos.size()` is 0 making `m_dynamic_reg_infos[pa_regnum + i - 1]` into `m_dynamic_reg_infos[0 + 0 - 1]` ?

I guess this could happen if SVE wasn't present but maybe you already account for that.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:326
+        m_dynamic_reg_infos[pa_regnum + i - 1].byte_size;
+    m_dynamic_reg_infos[pa_regnum + i].kinds[4] = pa_regnum + i;
+  }
----------------
Please comment what the [4] is getting.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:330
+  m_dynamic_reg_sets.push_back(g_reg_set_pauth_arm64);
+  m_dynamic_reg_sets[m_dynamic_reg_sets.size() - 1].registers =
+      pauth_regnum_collection.data();
----------------
You could simplify this with back (https://www.cplusplus.com/reference/vector/vector/back/) assuming this is a vector-ish type.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:336
+  uint32_t mte_regnum = m_dynamic_reg_infos.size();
+  for (uint32_t i = 0; i < 1; i++) {
+    m_mte_regnum_collection.push_back(mte_regnum + i);
----------------
Same as above

Given that there's only one MTE register you could also remove the for since we'll only ever have i==0. If we want to be nice and handle future expansion some MTERegs.size() would be preferred.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:434
+bool RegisterInfoPOSIX_arm64::IsPAuthReg(unsigned reg) const {
+  if (std::find(pauth_regnum_collection.begin(), pauth_regnum_collection.end(),
+                reg) != pauth_regnum_collection.end())
----------------
```
return std::find(pauth_regnum_collection.begin(), pauth_regnum_collection.end(),
                reg) != pauth_regnum_collection.end();
```


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:440
+
+bool RegisterInfoPOSIX_arm64::IsMTEReg(unsigned reg) const {
+  if (std::find(m_mte_regnum_collection.begin(), m_mte_regnum_collection.end(),
----------------
as above


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h:537
+// Defines pointer authentication mask registers
+#define DEFINE_PAUTH_REGS(reg)                                                 \
+  {                                                                            \
----------------
`DEFINE_PAUTH_REGS` is an odd name considering we use it for MTE too. `DEFINE_EXTENSION_REGS` ?

Also I would make it singular "REG"


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfos_arm64_sve.h:64
 
-  sve_ffr,
+  sve_ffr
 };
----------------
This seems unrelated


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

https://reviews.llvm.org/D96460