[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) {
----------------
omjavaid wrote:
> 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.
Cool, please add a comment in both places to that effect.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:543
+  bool contains_sve_reg_data =
+      (data_sp->GetByteSize() > (reg_data_min_size + GetSVEHeaderSize()));
+
----------------
omjavaid wrote:
> omjavaid wrote:
> > 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.
> Just to shed more light on this if SVE is note enabled we just have GPR + FPR + MTE predictable from size. As MTE is just 8 bytes we can be sure if checkpoint has data beyond FPR which is greater than GPR + FPR + SVE_HEADER that means we atleast have SVE header present. Then we check if its a valid header using !sve_vl_valid(m_sve_header.vl)) check and write it to configure SVE registers size before writing actual registers.
> If we dont have register data beyond greater than GPR+FPR+MTE then we can safely say there is no SVE present at all.
Right, of course because PAC is read only we only care about MTE and we know that the SVE header is > 8 bytes. Sounds good to me.

Another comment with that explanation please.


================
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());
----------------
omjavaid wrote:
> 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.
Got it.


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

https://reviews.llvm.org/D108739