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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
