On Thu, 28 Aug 2025 at 23:28, Richard Henderson
<richard.hender...@linaro.org> wrote:
>
> On 8/29/25 00:47, Peter Maydell wrote:
> > On Thu, 28 Aug 2025 at 13:10, Richard Henderson
> > <richard.hender...@linaro.org> wrote:
> >> void elf_core_copy_regs(target_elf_gregset_t *r, const CPUARMState *env)
> >> {
> >> for (int i = 0; i < 16; ++i) {
> >> - r->regs[i] = tswapreg(env->regs[i]);
> >> + r->pt.regs[i] = tswapal(env->regs[i]);
> >> }
> >> - r->regs[16] = tswapreg(cpsr_read((CPUARMState *)env));
> >> - r->regs[17] = tswapreg(env->regs[0]); /* XXX */
> >> + r->pt.cpsr = tswapal(cpsr_read((CPUARMState *)env));
> >> + r->pt.orig_r0 = tswapal(env->regs[0]);
> >
> > Why is it OK to drop the "XXX" comment here ?
>
> I assumed XXX meant "what is this", and the answer is orig_r0.
> I'm not even sure the value is wrong as-is, due to the way we process
> syscalls.
I suspect the XXX is probably because the original author
was unsure why this was here -- after all we've already
put env->regs[0] into r->pt.regs[0], so why have the
extra field if it never has a different value?
Compare the "FIXME" comment in the m68k elf_core_copy_regs(),
and contrast the way our x86 code is explicitly
putting some other value in orig_ax. Are these different
kinds of orig_foo, or are we doing unnecessary work on
x86, or missing something for arm and m68k?
If it is OK to use env->regs[0] here we could probably
use a comment explaining why the struct field exists
and why our implementation differs from the kernel in
a way that makes the two fields always have the same value.
thanks
-- PMM
