Re: [PATCH] [Arm] Remove dead FPA code

[Luis Machado <luis.machado@arm.com>]

On 10/4/22 22:36, John Baldwin wrote:
> On 10/4/22 10:43 AM, Luis Machado wrote:
>> On 10/4/22 18:08, John Baldwin wrote:
>>> On 10/4/22 1:43 AM, Luis Machado via Gdb-patches wrote:
>>>> On 10/3/22 20:16, Pedro Alves wrote:
>>>>> On 2022-09-20 1:30 p.m., Luis Machado via Gdb-patches wrote:
>>>>>
>>>>>> diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h
>>>>>> index 36757493406..74a6ba93bc7 100644
>>>>>> --- a/gdb/arch/arm.h
>>>>>> +++ b/gdb/arch/arm.h
>>>>>> @@ -44,11 +44,6 @@ enum gdb_regnum {
>>>>>>       ARM_SP_REGNUM = 13,        /* Contains address of top of stack */
>>>>>>       ARM_LR_REGNUM = 14,        /* address to return to from a function call */
>>>>>>       ARM_PC_REGNUM = 15,        /* Contains program counter */
>>>>>> -  /* F0..F7 are the fp registers for the (obsolete) FPA architecture.  */
>>>>>
>>>>> Shouldn't we leave behind a comment explaining why there's a hole between 15 and 25?
>>>>
>>>> I pondered about this a bit more, and I think we should close the gap and bring CPSR down to
>>>> 16, its "natural" position. It is what linux uses for user_regs as well, in gdb/arch/arm-linux.h:
>>>>
>>>> /* The index to access CSPR in user_regs defined in GLIBC.  */
>>>> #define ARM_CPSR_GREGNUM 16
>>>>
>>>>>
>>>>> IIRC the numbers can't be changed since we need to handle the case when the target
>>>>> doesn't send an xml tdesc, so it'd be good to help future readers understand why
>>>>> there's a hole.
>>>>
>>>> That's correct. Though a 32-bit Arm target that doesn't support XML descriptions these days is not very
>>>> common. I haven't seen one in a while.
>>>>
>>>> I'm willing to declare old 32-bit Arm targets that don't send XML target descriptions back as unsupported.
>>>>
>>>> To that effect, I suppose we should add a note to make it more explicit.
>>>>
>>>> More below.
>>>
>>> FWIW, the GDB stub in FreeBSD's kernel does not use XML target descriptions
>>> for any architectures, but it also only tends to do GPRs and not any floating
>>> point.  For 32-bit ARM it does not report any register values higher than
>>> number 15 (PC), so it would not be affected by changing this.
>>
>> Does it care about CPSR and/or XPSR? Could you please give it a try to see if the defaults would suit it just fine?
> 
> Hmm, I misread and it does care about CPSR for the current thread.  The
> relevant code is here (From https://cgit.freebsd.org/src/tree/sys/arm/arm/gdb_machdep.c):
> 
> void *
> gdb_cpu_getreg(int regnum, size_t *regsz)
> {
> 
>      *regsz = gdb_cpu_regsz(regnum);
> 
>      if (kdb_thread == curthread) {
>          if (regnum < 13)
>              return (&kdb_frame->tf_r0 + regnum);
>          if (regnum == 13)
>              return (&kdb_frame->tf_svc_sp);
>          if (regnum == 14)
>              return (&kdb_frame->tf_svc_lr);
>          if (regnum == 15)
>              return (&kdb_frame->tf_pc);
>          if (regnum == 25)
>              return (&kdb_frame->tf_spsr);
>      }
> 
>      switch (regnum) {
>      case 4:  return (&kdb_thrctx->pcb_regs.sf_r4);
>      case 5:  return (&kdb_thrctx->pcb_regs.sf_r5);
>      case 6:  return (&kdb_thrctx->pcb_regs.sf_r6);
>      case 7:  return (&kdb_thrctx->pcb_regs.sf_r7);
>      case 8:  return (&kdb_thrctx->pcb_regs.sf_r8);
>      case 9:  return (&kdb_thrctx->pcb_regs.sf_r9);
>      case 10:  return (&kdb_thrctx->pcb_regs.sf_r10);
>      case 11:  return (&kdb_thrctx->pcb_regs.sf_r11);
>      case 12:  return (&kdb_thrctx->pcb_regs.sf_r12);
>      case 13:  stacktest = kdb_thrctx->pcb_regs.sf_sp + 5 * 4;
>            return (&stacktest);
>      case 15:
>            /*
>             * On context switch, the PC is not put in the PCB, but
>             * we can retrieve it from the stack.
>             */
>            if (kdb_thrctx->pcb_regs.sf_sp > KERNBASE) {
>                kdb_thrctx->pcb_regs.sf_pc = *(register_t *)
>                    (kdb_thrctx->pcb_regs.sf_sp + 4 * 4);
>                return (&kdb_thrctx->pcb_regs.sf_pc);
>            }
>      }
> 
>      return (NULL);
> }
> 
> The 'kdb_thread == curthread' case is when a thread enters the debugger due
> to a crash or breakpoint, etc.  We do return CPSR for that thread, but we do
> not return it for other threads.  It looks like we do also know the FPA register
> size so that we return enough "xx" bytes in the 'g' reply to mark the FP
> registers as unavailable so that we can return the value of CPSR in the 'g'
> reply.
> 
>  From https://cgit.freebsd.org/src/tree/sys/arm/include/gdb_machdep.h:
> 
> #define    GDB_NREGS    26
> #define    GDB_REG_SP    13
> #define    GDB_REG_LR    14
> #define    GDB_REG_PC    15
> 
> static __inline size_t
> gdb_cpu_regsz(int regnum)
> {
>      /*
>       * GDB expects the FPA registers f0-f7, each 96 bits wide, to be placed
>       * in between the PC and CSPR in response to a "g" packet.
>       */
>      return (regnum >= 16 && regnum <= 23 ? 12 : sizeof(int));
> }
> 
> NetBSD's kernel seems to have similar knowledge:
> 
> http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/arch/arm/include/db_machdep.h?rev=1.28&content-type=text/x-cvsweb-markup&only_with_tag=MAIN
> 
> (The kgdb bits near the bottom)
> 
> Linux's kernel also seems to maybe hardcode this knowledge as well:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/kernel/kgdb.c#n21
> 

Yeah, that's what I was worried about. Register discoveries without XML are not great, and more recently debugging stubs have been
exposing more system registers. Having to consider FPA (which was *removed* 10 years ago from GCC, but fell in disuse before then) is not
acceptable at this point.

If those debugging stubs want to skip XML, I think it would be reasonable for them to at least update the expected 'g' packet to contain just
the basic registers, with CPSR as 16.

That might need some coordination. I can coordinate this from the Linux Kernel's side, but I never dealt with the BSD kernels.