Skip to content

i#7046 memory dump: add missing pstate value to aarch64 user_regs_struct. #7257

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ivankyluk wants to merge 1 commit into
base: master
Choose a base branch
from
Open

i#7046 memory dump: add missing pstate value to aarch64 user_regs_struct. #7257

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/unix/coredump.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

@ivankyluk ivankyluk Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@derekbruening derekbruening Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@derekbruening derekbruening Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#else
# error Unsupported architecture
#endif
Expand Down
Loading