[Lldb-commits] [PATCH] D108739: [LLDB] AArch64 SVE restore SVE registers after expression

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:451
 
 Status NativeRegisterContextLinux_arm64::ReadAllRegisterValues(
     lldb::DataBufferSP &data_sp) {
----------------
Do we need PAC registers here too? (and in write values)


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:543
+  bool contains_sve_reg_data =
+      (data_sp->GetByteSize() > (reg_data_min_size + GetSVEHeaderSize()));
+
----------------
I'm a bit dubious about these size checks as a way to tell if things are present, should this one have `GetRegisterInfo().IsSVEEnabled()` also?

I'm thinking what if PAC plus MTE plus <random future ext> adds enough registers to exceed the size of the SVE header, but the process doesn't in fact have SVE.

You could do the size checks in terms of bytes left to read from the data_sp, that might simplify it some too.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:587
 
+  if (GetRegisterInfo().IsMTEEnabled() && data_sp->GetByteSize() > reg_data_min_size) {
+    ::memcpy(GetMTEControl(), src, GetMTEControlSize());
----------------
This could go wrong if data_sp contains GPR+FPR+SVE data and somehow the register info thinks MTE is enabled. Not sure if that's a realistic thing to handle here.

If you did this in terms of bytes left you could check that there are bytes left to read from data_sp by this point.

(it's a shame the data_sp we get isn't some richer structure we could ask for offsets into)


================
Comment at: lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/TestSVERegisters.py:175
+        # We called a jitted function above which must not have changed SVE
+        # vector length or regiter values.
+        self.check_sve_regs_read(z_reg_size)
----------------
regiter -> register


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

https://reviews.llvm.org/D108739