Re: [PATCH 2/5] backends: Add set_initial_registers_tid callback for LoongArch

[Youling Tang <tangyouling@loongson.cn>]

On 05/09/2023 09:32 AM, Youling Tang wrote:
> Hi, Mark
>
> On 05/09/2023 03:57 AM, Mark Wielaard wrote:
>> Him
>>
>> On Fri, Apr 07, 2023 at 10:59:25AM +0800, Youling Tang wrote:
>>> This patch implements the set_initial_registers_tid hook for LoongArch.
>>
>> Looks good, but one question:
>>
>>> +  /* Floating-point registers (only 64bits are used).  */
>>> +  struct user_fp_struct fregs;
>>> +  iovec.iov_base = &fregs;
>>> +  iovec.iov_len = sizeof (fregs);
>>> +  if (ptrace (PTRACE_GETREGSET, tid, NT_FPREGSET, &iovec) != 0)
>>> +    return false;
>>> +
>>> +  Dwarf_Word dwarf_fregs[32];
>>> +  for (int r = 0; r < 32; r++)
>>> +    dwarf_fregs[r] = fregs.fpr[r] & 0xFFFFFFFF;
>>
>> The comment says 64bits, but the mask is for 32bits.
>> I assume the comment is wrong and the masking is deliberate?
>
> Thanks for pointing out that there is no need to mask the lower 32 bits
> here.
>
> It can be modified as follows,
> --- a/backends/loongarch_initreg.c
> +++ b/backends/loongarch_initreg.c
> @@ -79,11 +79,8 @@ loongarch_set_initial_registers_tid (pid_t tid
> __attribute__ ((unused)),
>    if (ptrace (PTRACE_GETREGSET, tid, NT_FPREGSET, &iovec) != 0)
>      return false;
>
> -  Dwarf_Word dwarf_fregs[32];
> -  for (int r = 0; r < 32; r++)
> -    dwarf_fregs[r] = fregs.fpr[r] & 0xFFFFFFFF;
> -
> -  if (! setfunc (32, 32, dwarf_fregs, arg))
> +  /* $f0-$f31 */
> +  if (! setfunc (32, 32, &fregs.fpr[0], arg))

Need to add `(Dwarf_Word *)`,

if (! setfunc (32, 32, (Dwarf_Word *) &fregs. fpr[0], arg))

>
> Do I need to send the v2 patchset again?
>
> Thanks,
> Youling.
>
>>
>> Cheers,
>>
>> Mark
>>