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 at 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?