public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Richard Earnshaw <Richard.Earnshaw@foss.arm.com>
To: John Baldwin <jhb@FreeBSD.org>,
	Luis Machado <luis.machado@arm.com>,
	Pedro Alves <pedro@palves.net>,
	gdb-patches@sourceware.org,
	David Spickett <David.Spickett@arm.com>
Subject: Re: [PATCH] [Arm] Remove dead FPA code
Date: Wed, 5 Oct 2022 17:57:53 +0100	[thread overview]
Message-ID: <c4fd7130-ce50-8a68-0004-a0c4d651b523@foss.arm.com> (raw)
In-Reply-To: <f92387e7-ab99-3694-66c1-9bd898f946ff@FreeBSD.org>



On 05/10/2022 17:48, John Baldwin wrote:
> On 10/5/22 1:26 AM, Luis Machado wrote:
>> 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.
> 
> I can certainly deal with updating FreeBSD.  I can try to find someone 
> over in NetBSD
> land.  Honestly, the simplest approach might be to start exporting XML 
> from the
> kernel.  However, an even simpler workaround for now might be having a 
> knob that
> can be set for older debugger binaries that still places CPSR at the old 
> register
> number vs the newer one.
> 

IMO, exporting an XML description of the register set(s) available is 
certainly the right way forward.  In fact, I'd strongly suggest we leave 
the existing packet formats as-is for now and add support for XML to the 
relevant kernels.  At some point then gdb (and LLDB) can simply stop 
working with older kernel versions that don't do it the 'right' way.

R.

  reply	other threads:[~2022-10-05 16:57 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-20 12:30 Luis Machado
2022-09-20 12:47 ` Eli Zaretskii
2022-10-02 13:39 ` Enze Li
2022-10-03  8:27   ` Luis Machado
2022-10-03 17:33 ` John Baldwin
2022-10-03 19:16 ` Pedro Alves
2022-10-04  8:43   ` Luis Machado
2022-10-04 17:08     ` John Baldwin
2022-10-04 17:43       ` Luis Machado
2022-10-04 21:36         ` John Baldwin
2022-10-05  8:26           ` Luis Machado
2022-10-05  8:36             ` David Spickett
2022-10-05  8:36               ` David Spickett
2022-10-05 16:48             ` John Baldwin
2022-10-05 16:57               ` Richard Earnshaw [this message]
2022-10-06 13:02                 ` Luis Machado
2022-10-10 14:58             ` Pedro Alves
2022-10-13  7:23               ` Luis Machado
2022-10-13  8:29                 ` Pedro Alves
2022-10-13  9:40                   ` Luis Machado
2022-10-25 13:54                     ` Luis Machado
2022-11-14 14:30                     ` Simon Marchi
2022-10-10 14:56     ` Pedro Alves
2022-10-13  7:18       ` Luis Machado
2022-10-13  8:44         ` Pedro Alves
2022-10-13  9:15           ` Luis Machado

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c4fd7130-ce50-8a68-0004-a0c4d651b523@foss.arm.com \
    --to=richard.earnshaw@foss.arm.com \
    --cc=David.Spickett@arm.com \
    --cc=gdb-patches@sourceware.org \
    --cc=jhb@FreeBSD.org \
    --cc=luis.machado@arm.com \
    --cc=pedro@palves.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).