Make use of the fact that target_elf_gregset_t is a proper structure.
Drop ELF_NREG, target_elf_greg_t, and tswapreg.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/arm/target_elf.h | 11 +++++++----
 linux-user/arm/elfload.c    |  8 +++-----
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/linux-user/arm/target_elf.h b/linux-user/arm/target_elf.h
index 94db3738e8..fa8f8af2f3 100644
--- a/linux-user/arm/target_elf.h
+++ b/linux-user/arm/target_elf.h
@@ -8,16 +8,19 @@
 #ifndef ARM_TARGET_ELF_H
 #define ARM_TARGET_ELF_H
 
+#include "target_ptrace.h"
+
 #define HAVE_ELF_HWCAP          1
 #define HAVE_ELF_HWCAP2         1
 #define HAVE_ELF_PLATFORM       1
 #define HAVE_ELF_CORE_DUMP      1
 
-typedef abi_ulong target_elf_greg_t;
-
-#define ELF_NREG                18
+/*
+ * See linux kernel: arch/arm/include/asm/elf.h, where
+ * elf_gregset_t is mapped to struct pt_regs via sizeof.
+ */
 typedef struct target_elf_gregset_t {
-    target_elf_greg_t regs[ELF_NREG];
+    struct target_pt_regs pt;
 } target_elf_gregset_t;
 
 #endif
diff --git a/linux-user/arm/elfload.c b/linux-user/arm/elfload.c
index 47fe16a1a6..726d3ec25c 100644
--- a/linux-user/arm/elfload.c
+++ b/linux-user/arm/elfload.c
@@ -201,13 +201,11 @@ const char *get_elf_platform(CPUState *cs)
 #undef END
 }
 
-#define tswapreg(ptr)   tswapal(ptr)
-
 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]);
 }
-- 
2.43.0

On Thu, 28 Aug 2025 at 13:10, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Make use of the fact that target_elf_gregset_t is a proper structure.
> Drop ELF_NREG, target_elf_greg_t, and tswapreg.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/arm/target_elf.h | 11 +++++++----
>  linux-user/arm/elfload.c    |  8 +++-----
>  2 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/linux-user/arm/target_elf.h b/linux-user/arm/target_elf.h
> index 94db3738e8..fa8f8af2f3 100644
> --- a/linux-user/arm/target_elf.h
> +++ b/linux-user/arm/target_elf.h
> @@ -8,16 +8,19 @@
>  #ifndef ARM_TARGET_ELF_H
>  #define ARM_TARGET_ELF_H
>
> +#include "target_ptrace.h"
> +
>  #define HAVE_ELF_HWCAP          1
>  #define HAVE_ELF_HWCAP2         1
>  #define HAVE_ELF_PLATFORM       1
>  #define HAVE_ELF_CORE_DUMP      1
>
> -typedef abi_ulong target_elf_greg_t;
> -
> -#define ELF_NREG                18
> +/*
> + * See linux kernel: arch/arm/include/asm/elf.h, where
> + * elf_gregset_t is mapped to struct pt_regs via sizeof.
> + */
>  typedef struct target_elf_gregset_t {
> -    target_elf_greg_t regs[ELF_NREG];
> +    struct target_pt_regs pt;
>  } target_elf_gregset_t;
>
>  #endif
> diff --git a/linux-user/arm/elfload.c b/linux-user/arm/elfload.c
> index 47fe16a1a6..726d3ec25c 100644
> --- a/linux-user/arm/elfload.c
> +++ b/linux-user/arm/elfload.c
> @@ -201,13 +201,11 @@ const char *get_elf_platform(CPUState *cs)
>  #undef END
>  }
>
> -#define tswapreg(ptr)   tswapal(ptr)
> -
>  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 ?

otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

On 8/29/25 00:47, Peter Maydell wrote:
> On Thu, 28 Aug 2025 at 13:10, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Make use of the fact that target_elf_gregset_t is a proper structure.
>> Drop ELF_NREG, target_elf_greg_t, and tswapreg.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   linux-user/arm/target_elf.h | 11 +++++++----
>>   linux-user/arm/elfload.c    |  8 +++-----
>>   2 files changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/linux-user/arm/target_elf.h b/linux-user/arm/target_elf.h
>> index 94db3738e8..fa8f8af2f3 100644
>> --- a/linux-user/arm/target_elf.h
>> +++ b/linux-user/arm/target_elf.h
>> @@ -8,16 +8,19 @@
>>   #ifndef ARM_TARGET_ELF_H
>>   #define ARM_TARGET_ELF_H
>>
>> +#include "target_ptrace.h"
>> +
>>   #define HAVE_ELF_HWCAP          1
>>   #define HAVE_ELF_HWCAP2         1
>>   #define HAVE_ELF_PLATFORM       1
>>   #define HAVE_ELF_CORE_DUMP      1
>>
>> -typedef abi_ulong target_elf_greg_t;
>> -
>> -#define ELF_NREG                18
>> +/*
>> + * See linux kernel: arch/arm/include/asm/elf.h, where
>> + * elf_gregset_t is mapped to struct pt_regs via sizeof.
>> + */
>>   typedef struct target_elf_gregset_t {
>> -    target_elf_greg_t regs[ELF_NREG];
>> +    struct target_pt_regs pt;
>>   } target_elf_gregset_t;
>>
>>   #endif
>> diff --git a/linux-user/arm/elfload.c b/linux-user/arm/elfload.c
>> index 47fe16a1a6..726d3ec25c 100644
>> --- a/linux-user/arm/elfload.c
>> +++ b/linux-user/arm/elfload.c
>> @@ -201,13 +201,11 @@ const char *get_elf_platform(CPUState *cs)
>>   #undef END
>>   }
>>
>> -#define tswapreg(ptr)   tswapal(ptr)
>> -
>>   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.


r~

On Thu, 28 Aug 2025 at 23:28, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/29/25 00:47, Peter Maydell wrote:
> > On Thu, 28 Aug 2025 at 13:10, Richard Henderson
> > <richard.henderson@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

On 8/29/25 18:35, Peter Maydell wrote:
> 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?
I really don't know the answer to that one.
I'll put a fixme back here for now.


r~