Page MenuHomePhabricator

[LLDB] AArch64 SVE restore SVE registers after expression
ClosedPublic

Authored by omjavaid on Aug 25 2021, 3:51 PM.

Details

Summary

This patch fixes register save/restore on expression call to also include SVE registers.

This will fix expression calls like:

re re p1

<Register Value P1 before expression>

p <var-name or function call>

re re p1

<Register Value P1 after expression>

In above example register P1 should remain the same before and after the expression evaluation.

Diff Detail

Event Timeline

omjavaid created this revision.Aug 25 2021, 3:51 PM
omjavaid requested review of this revision.Aug 25 2021, 3:51 PM
Matt added a subscriber: Matt.Aug 27 2021, 5:43 AM
DavidSpickett added inline comments.Sep 1 2021, 7:24 AM
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
451

Do we need PAC registers here too? (and in write values)

543

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.

587

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)

lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/TestSVERegisters.py
175

regiter -> register

omjavaid added inline comments.Sep 1 2021, 8:10 AM
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
451

We implement PAC as read only so we dont wanna save them either.

543

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.

587

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.

lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/TestSVERegisters.py
175

Ack.

omjavaid added inline comments.Sep 1 2021, 8:20 AM
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
543

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.

I have tested this patch with my setup and it fixes the issues described. Thanks.

DavidSpickett added inline comments.Sep 7 2021, 3:56 AM
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
451

Cool, please add a comment in both places to that effect.

543

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.

587

Got it.

Hopefully in the future we can get some kind of smarter container for this register data in future but this looks good for now with a few extra comments for the logic of it. (and so we know where to make changes later)

omjavaid updated this revision to Diff 371380.Sep 8 2021, 9:59 AM

This update addresses review comments.

DavidSpickett accepted this revision.Sep 9 2021, 1:37 AM

LGTM with the clang-format warning fixed.

This revision is now accepted and ready to land.Sep 9 2021, 1:37 AM
This revision was landed with ongoing or failed builds.Sep 9 2021, 4:07 AM
This revision was automatically updated to reflect the committed changes.