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

Tue Feb 16 08:38:28 PST 2021

omjavaid added a comment.

Subsequent update will resolve review comments. @DavidSpickett  thanks for review. much appreciated

================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:79

+  m_mte_ctrl_reg = 0;
+
----------------
DavidSpickett wrote:
> Should m_pac_mask also be initialised or is it already?
Ack. Forgot to do it. 

================
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;
----------------
DavidSpickett wrote:
> 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.
Agreed

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

================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:1100
   m_sve_header_is_valid = false;
+  m_pac_mask_is_valid = false;

----------------
DavidSpickett wrote:
> I think `m_mte_ctrl_is_valid` is missing here.
Ack.

================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:1198
+
+  if (m_mte_ctrl_is_valid)
+    return error;
----------------
DavidSpickett wrote:
> 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.
Status is used to hold error codes and default error code is 'success'. If we have a valid cache value that means we dont need to read and return success thats why returning error without setting the error code.

If we need to read then we call ReadRegisterSet which will set the error code in case of failure.

================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:1216
+
+  error = ReadMTEControl();
+  if (error.Fail())
----------------
DavidSpickett wrote:
> This is a sanity check that we didn't ask to write this register on a system that doesn't have it?
This may only be relevant for optional regsets where we need to check if registers are readable before writing and registers which need partial modification and we need to have most recent value.

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

================
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);
----------------
DavidSpickett wrote:
> 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?
k_num_pauth_register already populated and will be used here as well.

================
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;
----------------
DavidSpickett wrote:
> 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.
Its not possible for m_dynamic_reg_infos.size to be zero because it is populated with either g_register_infos_arm64_sve_le or g_register_infos_arm64_le register infos array.

================
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;
+  }
----------------
DavidSpickett wrote:
> Please comment what the [4] is getting.
Ack.

================
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();
----------------
DavidSpickett wrote:
> You could simplify this with back (https://www.cplusplus.com/reference/vector/vector/back/) assuming this is a vector-ish type.
done.

================
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);
----------------
DavidSpickett wrote:
> 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.
Ack.

================
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())
----------------
DavidSpickett wrote:
> ```
> return std::find(pauth_regnum_collection.begin(), pauth_regnum_collection.end(),
>                 reg) != pauth_regnum_collection.end();
> ```
Ack.

================
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(),
----------------
DavidSpickett wrote:
> as above
Ack.

================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h:537
+// Defines pointer authentication mask registers
+#define DEFINE_PAUTH_REGS(reg)                                                 \
+  {                                                                            \
----------------
DavidSpickett wrote:
> `DEFINE_PAUTH_REGS` is an odd name considering we use it for MTE too. `DEFINE_EXTENSION_REGS` ?
> 
> Also I would make it singular "REG"
Agreed.

================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfos_arm64_sve.h:64

-  sve_ffr,
+  sve_ffr
 };
----------------
DavidSpickett wrote:
> This seems unrelated
Ack.

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

https://reviews.llvm.org/D96460