i#7046 memory dump: add missing pstate value to aarch64 user_regs_struct. #7257
Conversation
| @@ -285,6 +285,7 @@ mcontext_to_user_regs(DR_PARAM_IN priv_mcontext_t *mcontext, | |||
| regs->regs[30] = mcontext->r30; | |||
| regs->sp = mcontext->sp; | |||
| regs->pc = (uint64_t)mcontext->pc; | |||
| regs->pstate = mcontext->nzcv; | |||
There was a problem hiding this comment.
Copy a review comment from #7255:
This isn't in the PR description: is this an unrelated bug fix? Should this be a separate PR? Should there be a test that verifies we included everything?
The user_regs_struct for aarch64 has the following fields:
struct user_regs_struct
{
unsigned long long regs[31];
unsigned long long sp;
unsigned long long pc;
unsigned long long pstate;
};
pstate is the only one missing.
To test whether this function copies all the field is straight forward but I have not been able to think of a way to test it functionally.
There was a problem hiding this comment.
At least explain how you tested in the PR description: how do you know nzcv is enough for pstate (see below)? Did you generate a core dump from the kernel at the same point in the same app and compare the fields manually? That sounds like something that should be done. That could conceivably be turned into an automated test but it could be tricky to get core dumps enabled in a test VM. At least describe your manual testing.
| @@ -285,6 +285,7 @@ mcontext_to_user_regs(DR_PARAM_IN priv_mcontext_t *mcontext, | |||
| regs->regs[30] = mcontext->r30; | |||
| regs->sp = mcontext->sp; | |||
| regs->pc = (uint64_t)mcontext->pc; | |||
| regs->pstate = mcontext->nzcv; | |||
There was a problem hiding this comment.
Doesn't the aarch64 term "pstate" include a bunch of other things and not just the NZCV flags? Are you sure this pstate field includes only NZCV? Please add a comment about this so it's clear.
| @@ -285,6 +285,7 @@ mcontext_to_user_regs(DR_PARAM_IN priv_mcontext_t *mcontext, | |||
| regs->regs[30] = mcontext->r30; | |||
| regs->sp = mcontext->sp; | |||
| regs->pc = (uint64_t)mcontext->pc; | |||
| regs->pstate = mcontext->nzcv; | |||
There was a problem hiding this comment.
At least explain how you tested in the PR description: how do you know nzcv is enough for pstate (see below)? Did you generate a core dump from the kernel at the same point in the same app and compare the fields manually? That sounds like something that should be done. That could conceivably be turned into an automated test but it could be tricky to get core dumps enabled in a test VM. At least describe your manual testing.
#7088 missed the pstate when copying the register values from priv_mcontext_t to user_regs_struct.
The user_regs_struct has the follow fields:
struct user_regs_struct
{
unsigned long long regs[31];
unsigned long long sp;
unsigned long long pc;
unsigned long long pstate;
};
for Aarch64. The rest of the fields have already been copied.
Issue: #7046