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