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

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added inline comments.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:451
 
 Status NativeRegisterContextLinux_arm64::ReadAllRegisterValues(
     lldb::DataBufferSP &data_sp) {
----------------
DavidSpickett wrote:
> Do we need PAC registers here too? (and in write values)
We implement PAC as read only so we dont wanna save them either.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:543
+  bool contains_sve_reg_data =
+      (data_sp->GetByteSize() > (reg_data_min_size + GetSVEHeaderSize()));
+
----------------
DavidSpickett wrote:
> 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.
We are not sure if SVE was enabled or not when we restore. So size checks is the only way we can figure out if a register checkpoint was created when SVE was enabled. This works for now but if we have to stack another register set which is greater than size of SVE header then we will have to put information about enabled register sets inside the checkpoint maybe in a bitmap describing which register set is enabled and look for those at predefined offsets. I thought this wasn't needed for now and will over complicate already complicated variable sized checkpoint.


================
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());
----------------
DavidSpickett wrote:
> 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)
FPR and SVE cannot live together when SVE is present FPR registers are replicated by first 128bits of SVE Z registers so we will either save FPR or SVE data not both.

reg_data_min_size can either be FPR + GPR or SVE + GPR thats why we calculate it again when data has SVE.


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


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

https://reviews.llvm.org/D108739