Re: [PATCH] gdb: LoongArch: Support LBT registers

[Kevin Buettner via Gdb-patches <gdb-patches@sourceware.org>]

I have one nit regarding an enum name...

On Fri, 25 Aug 2023 18:11:13 +0800
Feiyang Chen <chenfeiyang@loongson.cn> wrote:


> diff --git a/gdb/arch/loongarch.h b/gdb/arch/loongarch.h
> index d0a63dc2ac5..73f3d64fb16 100644
> --- a/gdb/arch/loongarch.h
> +++ b/gdb/arch/loongarch.h
> @@ -41,6 +41,11 @@ enum loongarch_regnum
>    LOONGARCH_FIRST_FCC_REGNUM = LOONGARCH_FIRST_FP_REGNUM + LOONGARCH_LINUX_NUM_FPREGSET,
>    LOONGARCH_LINUX_NUM_FCC = 8,
>    LOONGARCH_FCSR_REGNUM = LOONGARCH_FIRST_FCC_REGNUM + LOONGARCH_LINUX_NUM_FCC,
> +  LOONGARCH_FIRST_SCR_REGNUM = LOONGARCH_FCSR_REGNUM + 1,
> +  LOONGARCH_LINUX_NUM_SCR = 4,
> +  LOONGARCH_SCR_REGNUM = LOONGARCH_FCSR_REGNUM + LOONGARCH_LINUX_NUM_SCR,

Instead of LOONGARCH_SCR_REGNUM, I recommend that it be named
LOONGARCH_LAST_SCR_REGNUM instead.  This fits in with its usage in the
range tests, e.g. this one from loongarch_supply_lbtregset:

> +  else if (regnum >= LOONGARCH_FIRST_SCR_REGNUM && regnum <= LOONGARCH_SCR_REGNUM)
> +    {
> +      buf = (const gdb_byte*) regs + scrsize * (regnum - LOONGARCH_FIRST_SCR_REGNUM);
> +      regcache->raw_supply (regnum, (const void *) buf);
> +    }

Naming it LOONGARCH_SCR_REGNUM suggested to me that the index in
question was for the (one and only) SCR register, but that's clearly
not the case since there are 4 of them.

Also, it appears to me that source code corresponding to some of the
lines in your patch will exceed 80 characters, so those should be
adjusted as well.  See:

https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Column_limits

Kevin