RFC: FPA support removal from gdb and its impact on kgdb

RFC: FPA support removal from gdb and its impact on kgdb

[Luis Machado]

Hi,

Following the removal of Arm FPA support from GCC in ~2012, I'm pursuing the same for gdb [1].

We should be able to remove mostly everything from gdb, but there is a small portion of code that
deals with targets (like kgdb) that don't expose their register sets through XML. This code in gdb
attempts to detect the register set based on the size of the register buffer ('g' remote packet).

The problem is that CPSR was historically hardcoded to register 25 to make way for the FPA registers in the middle.

 From arch/arm/kernel/kgdb.c:

struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] =
{
         { "r0", 4, offsetof(struct pt_regs, ARM_r0)},
         { "r1", 4, offsetof(struct pt_regs, ARM_r1)},
         { "r2", 4, offsetof(struct pt_regs, ARM_r2)},
         { "r3", 4, offsetof(struct pt_regs, ARM_r3)},
         { "r4", 4, offsetof(struct pt_regs, ARM_r4)},
         { "r5", 4, offsetof(struct pt_regs, ARM_r5)},
         { "r6", 4, offsetof(struct pt_regs, ARM_r6)},
         { "r7", 4, offsetof(struct pt_regs, ARM_r7)},
         { "r8", 4, offsetof(struct pt_regs, ARM_r8)},
         { "r9", 4, offsetof(struct pt_regs, ARM_r9)},
         { "r10", 4, offsetof(struct pt_regs, ARM_r10)},
         { "fp", 4, offsetof(struct pt_regs, ARM_fp)},
         { "ip", 4, offsetof(struct pt_regs, ARM_ip)},
         { "sp", 4, offsetof(struct pt_regs, ARM_sp)},
         { "lr", 4, offsetof(struct pt_regs, ARM_lr)},
         { "pc", 4, offsetof(struct pt_regs, ARM_pc)},
         { "f0", 12, -1 },
         { "f1", 12, -1 },
         { "f2", 12, -1 },
         { "f3", 12, -1 },
         { "f4", 12, -1 },
         { "f5", 12, -1 },
         { "f6", 12, -1 },
         { "f7", 12, -1 },
         { "fps", 4, -1 },
         { "cpsr", 4, offsetof(struct pt_regs, ARM_cpsr)},
};

Changing gdb to use CPSR as register 16 (not 25) would potentially break Linux's kgdb (and also
*BSD's kgdb). Unless these -1 offsets get handled in a special way and the f<n> registers never
make their way to the register buffer in the 'g'/'G' packets.

I haven't tried to connect to a 32-bit kgdb, so I'm not sure. Could someone confirm?

But if the 'g'/'G' packets do include f<n> entries, then the FPA removal patch will cause issues for kgdb.

There are 3 solutions that I'd like to get feedback on:

(1) - Apply the gdb patch as-is and update kgdb to send XML register descriptions.

gdb has been using XML for target descriptions for a long while now. kgdb would just need to send the
hardcoded XML string. Older kgdb's wouldn't work with newer gdb's. Newer kgdb's would work with both old
and new gdb's.


(2) - Apply the gdb patch as-is and update kgdb to drop the f<n> registers.

This means kgdb needs to be updated to work with a patched/newer gdb. Older gdb's could be used with
older kgdb's. Newer kgdb's would require new gdb's.


(3) - Remove FPA from gdb, but keep a limited-time compatibility layer for gdb 13 (not released yet) and
remove it in gdb 14.

This should give kgdb's enough time to get updated with (1) or (2). Older kgdb's would require older
gdb's (<= 13).

Thoughts?

[1] https://sourceware.org/pipermail/gdb-patches/2022-September/191951.html

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: RFC: FPA support removal from gdb and its impact on kgdb

[Russell King (Oracle)]

On Wed, Oct 05, 2022 at 04:03:04PM +0100, Luis Machado wrote:
> Hi,
> 
> Following the removal of Arm FPA support from GCC in ~2012, I'm pursuing the same for gdb [1].
> 
> We should be able to remove mostly everything from gdb, but there is a small portion of code that
> deals with targets (like kgdb) that don't expose their register sets through XML. This code in gdb
> attempts to detect the register set based on the size of the register buffer ('g' remote packet).
> 
> The problem is that CPSR was historically hardcoded to register 25 to make way for the FPA registers in the middle.
> 
> From arch/arm/kernel/kgdb.c:
> 
> struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] =
> {
>         { "r0", 4, offsetof(struct pt_regs, ARM_r0)},
>         { "r1", 4, offsetof(struct pt_regs, ARM_r1)},
>         { "r2", 4, offsetof(struct pt_regs, ARM_r2)},
>         { "r3", 4, offsetof(struct pt_regs, ARM_r3)},
>         { "r4", 4, offsetof(struct pt_regs, ARM_r4)},
>         { "r5", 4, offsetof(struct pt_regs, ARM_r5)},
>         { "r6", 4, offsetof(struct pt_regs, ARM_r6)},
>         { "r7", 4, offsetof(struct pt_regs, ARM_r7)},
>         { "r8", 4, offsetof(struct pt_regs, ARM_r8)},
>         { "r9", 4, offsetof(struct pt_regs, ARM_r9)},
>         { "r10", 4, offsetof(struct pt_regs, ARM_r10)},
>         { "fp", 4, offsetof(struct pt_regs, ARM_fp)},
>         { "ip", 4, offsetof(struct pt_regs, ARM_ip)},
>         { "sp", 4, offsetof(struct pt_regs, ARM_sp)},
>         { "lr", 4, offsetof(struct pt_regs, ARM_lr)},
>         { "pc", 4, offsetof(struct pt_regs, ARM_pc)},
>         { "f0", 12, -1 },
>         { "f1", 12, -1 },
>         { "f2", 12, -1 },
>         { "f3", 12, -1 },
>         { "f4", 12, -1 },
>         { "f5", 12, -1 },
>         { "f6", 12, -1 },
>         { "f7", 12, -1 },
>         { "fps", 4, -1 },
>         { "cpsr", 4, offsetof(struct pt_regs, ARM_cpsr)},
> };
> 
> Changing gdb to use CPSR as register 16 (not 25) would potentially break Linux's kgdb (and also
> *BSD's kgdb). Unless these -1 offsets get handled in a special way and the f<n> registers never
> make their way to the register buffer in the 'g'/'G' packets.

Looking at the code in the file you mention above, specifically
dbg_get_reg() which is immediately below the table you quoted above,
we see that an attempt to get the FPA registers (with an offset of
-1) will result in a value of all-zeros returned.

If we look further into the gdbstub that the kernel uses (in
kernel/debug/gdbstub.c) we find:

void pt_regs_to_gdb_regs(unsigned long *gdb_regs, struct pt_regs *regs)
{
        int i;
        int idx = 0;
        char *ptr = (char *)gdb_regs;

        for (i = 0; i < DBG_MAX_REG_NUM; i++) {
                dbg_get_reg(i, ptr + idx, regs);
                idx += dbg_reg_def[i].size;
        }
}

So, all registers are fetched to a block of memory (defined by the first
argument to this function). This is called from gdb_get_regs_helper()
and places the register values in a static-global gdb_regs array.

And then we have:

/* Handle the 'g' get registers request */
static void gdb_cmd_getregs(struct kgdb_state *ks)
{
        gdb_get_regs_helper(ks);
        kgdb_mem2hex((char *)gdb_regs, remcom_out_buffer, NUMREGBYTES);
}

So, it looks to me like the stub returns the registers as a block of
hex, and removing the FPA registers _will_ break the stub.

Given this, and this is a fundamental interface between two different
pieces of software, I fail to see how you can even consider removing
support for this - if you remove support from gdb, then later gdb
will be unable to debug existing kernels.

In kernel-land, we have a rule: don't break userspace. I think there
should also be a rule for userspace: don't break the kernel!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: RFC: FPA support removal from gdb and its impact on kgdb

[Luis Machado]

Hi Russel,

On 10/5/22 17:54, Russell King (Oracle) wrote:
> On Wed, Oct 05, 2022 at 04:03:04PM +0100, Luis Machado wrote:
>> Hi,
>>
>> Following the removal of Arm FPA support from GCC in ~2012, I'm pursuing the same for gdb [1].
>>
>> We should be able to remove mostly everything from gdb, but there is a small portion of code that
>> deals with targets (like kgdb) that don't expose their register sets through XML. This code in gdb
>> attempts to detect the register set based on the size of the register buffer ('g' remote packet).
>>
>> The problem is that CPSR was historically hardcoded to register 25 to make way for the FPA registers in the middle.
>>
>>  From arch/arm/kernel/kgdb.c:
>>
>> struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] =
>> {
>>          { "r0", 4, offsetof(struct pt_regs, ARM_r0)},
>>          { "r1", 4, offsetof(struct pt_regs, ARM_r1)},
>>          { "r2", 4, offsetof(struct pt_regs, ARM_r2)},
>>          { "r3", 4, offsetof(struct pt_regs, ARM_r3)},
>>          { "r4", 4, offsetof(struct pt_regs, ARM_r4)},
>>          { "r5", 4, offsetof(struct pt_regs, ARM_r5)},
>>          { "r6", 4, offsetof(struct pt_regs, ARM_r6)},
>>          { "r7", 4, offsetof(struct pt_regs, ARM_r7)},
>>          { "r8", 4, offsetof(struct pt_regs, ARM_r8)},
>>          { "r9", 4, offsetof(struct pt_regs, ARM_r9)},
>>          { "r10", 4, offsetof(struct pt_regs, ARM_r10)},
>>          { "fp", 4, offsetof(struct pt_regs, ARM_fp)},
>>          { "ip", 4, offsetof(struct pt_regs, ARM_ip)},
>>          { "sp", 4, offsetof(struct pt_regs, ARM_sp)},
>>          { "lr", 4, offsetof(struct pt_regs, ARM_lr)},
>>          { "pc", 4, offsetof(struct pt_regs, ARM_pc)},
>>          { "f0", 12, -1 },
>>          { "f1", 12, -1 },
>>          { "f2", 12, -1 },
>>          { "f3", 12, -1 },
>>          { "f4", 12, -1 },
>>          { "f5", 12, -1 },
>>          { "f6", 12, -1 },
>>          { "f7", 12, -1 },
>>          { "fps", 4, -1 },
>>          { "cpsr", 4, offsetof(struct pt_regs, ARM_cpsr)},
>> };
>>
>> Changing gdb to use CPSR as register 16 (not 25) would potentially break Linux's kgdb (and also
>> *BSD's kgdb). Unless these -1 offsets get handled in a special way and the f<n> registers never
>> make their way to the register buffer in the 'g'/'G' packets.
> 
> Looking at the code in the file you mention above, specifically
> dbg_get_reg() which is immediately below the table you quoted above,
> we see that an attempt to get the FPA registers (with an offset of
> -1) will result in a value of all-zeros returned.
> 
> If we look further into the gdbstub that the kernel uses (in
> kernel/debug/gdbstub.c) we find:
> 
> void pt_regs_to_gdb_regs(unsigned long *gdb_regs, struct pt_regs *regs)
> {
>          int i;
>          int idx = 0;
>          char *ptr = (char *)gdb_regs;
> 
>          for (i = 0; i < DBG_MAX_REG_NUM; i++) {
>                  dbg_get_reg(i, ptr + idx, regs);
>                  idx += dbg_reg_def[i].size;
>          }
> }
> 
> So, all registers are fetched to a block of memory (defined by the first
> argument to this function). This is called from gdb_get_regs_helper()
> and places the register values in a static-global gdb_regs array.
> 
> And then we have:
> 
> /* Handle the 'g' get registers request */
> static void gdb_cmd_getregs(struct kgdb_state *ks)
> {
>          gdb_get_regs_helper(ks);
>          kgdb_mem2hex((char *)gdb_regs, remcom_out_buffer, NUMREGBYTES);
> }
> 
> So, it looks to me like the stub returns the registers as a block of
> hex, and removing the FPA registers _will_ break the stub.

Right, that was my assumption.

> 
> Given this, and this is a fundamental interface between two different
> pieces of software, I fail to see how you can even consider removing
> support for this - if you remove support from gdb, then later gdb
> will be unable to debug existing kernels.

It is being considered because not even the Linux kernel uses these registers anymore. The kgdb
implementation was based on what GDB exposed long ago. Those registers didn't exist back then when
the kgdb code for Arm was put together:

commit 5cbad0ebf45c5417104b383dc0e34f64fa7f2473
Author: Jason Wessel <jason.wessel@windriver.com>
Date:   Wed Feb 20 13:33:40 2008 -0600

So it is not unreasonable to clean this up and reduce the maintenance burden on GDB's side.

Besides, GDB has moved to XML target descriptions around 2007, and it's been the optimal way to
transmit the register set layout from a debugging stub to GDB.

> 
> In kernel-land, we have a rule: don't break userspace. I think there
> should also be a rule for userspace: don't break the kernel!
> 

That's fair, and I'm not seeking to proactively break the kernel. That's why I listed some
possible solutions.

The best one is to update kgdb to send back XML data and drop the fp registers. On GDB's side,
we can carry compatibility code that will guarantee things will work until it makes sense
to remove the compatibility layer altogether.

Does that sound like a good compromise?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: RFC: FPA support removal from gdb and its impact on kgdb

[Russell King (Oracle)]

On Thu, Oct 06, 2022 at 02:42:09PM +0100, Luis Machado wrote:
> Hi Russel,
> 
> On 10/5/22 17:54, Russell King (Oracle) wrote:
> > Given this, and this is a fundamental interface between two different
> > pieces of software, I fail to see how you can even consider removing
> > support for this - if you remove support from gdb, then later gdb
> > will be unable to debug existing kernels.
> 
> It is being considered because not even the Linux kernel uses these
> registers anymore. The kgdb implementation was based on what GDB exposed
> long ago.

The kernel has never used the FPA not even an emulation of the FPA. The
kernel has provided a number of FPA emulation options over the years for
userspace to use, but never for its own use.

So kgdb's inclusion if these is merely to make kgdb's stub work with
gdb and nothing else.

> > In kernel-land, we have a rule: don't break userspace. I think there
> > should also be a rule for userspace: don't break the kernel!
> 
> That's fair, and I'm not seeking to proactively break the kernel. That's
> why I listed some possible solutions.
> 
> The best one is to update kgdb to send back XML data and drop the fp
> registers. On GDB's side, we can carry compatibility code that will
> guarantee things will work until it makes sense to remove the
> compatibility layer altogether.
> 
> Does that sound like a good compromise?

Yes, thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel