Re: [PATCH v2 3/3] gdbserver: Add RISC-V/Linux support

[Jim Wilson <jimw@sifive.com>]

On Wed, Jan 29, 2020 at 10:14 AM Maciej W. Rozycki <macro@wdc.com> wrote:
>         gdb/
>         * arch/riscv.h (riscv_create_target_description): Remove `const'
>         qualifier from the return type.
>         * arch/riscv.c (riscv_create_target_description): Likewise.
>         * nat/riscv-linux-tdesc.h (riscv_linux_read_description):
>         Likewise.
>         * nat/riscv-linux-tdesc.c (riscv_linux_read_description):
>         Likewise.
>         * configure.tgt <riscv*-*-linux*>: Set build_gdbserver=yes.
>
>         gdb/gdbserver/
>         * linux-riscv-low.c: New file.
>         * Makefile.in (SFILES): Add linux-riscv-low.c, arch/riscv.c, and
>         nat/riscv-linux-tdesc.c.
>         * configure.srv <riscv*-*-linux*> (srv_tgtobj)
>         (srv_linux_regsets, srv_linux_usrregs, srv_linux_thread_db):
>         Define.

Looks good to me, though I'm not an official review.

I noticed on the gdbserver console I'm getting a lot of ptrace warnings.
    ptrace(regsets_fetch_inferior_registers) PID=1678103: Invalid argument
    Warning: ptrace(regsets_store_inferior_registers): Invalid argument
This looks like a side effect of having two FP regsets defined, it
tries the first one, fails, and then tries the second one which is
correct.  If you mark them as OPTIONAL_REGS we would only get the
warning once which would be OK, except that they can't be both FP_REGS
and OPTIONAL_REGS at the same time.  I don't know what if anything
would break if they aren't marked as FP_REGS.   I only see explicit
checks for GENERAL_REGS and OPTIONAL_REGS; I don't see any checks for
FP_REGS.  Anyways, I would suggest as a future improvement that the
linux gdbserver regset support be extended so that a regset can be
marked as both FP_REGS and OPTIONAL_REGS.

There are some new functions and structures that don't have
explanatory comments before them, but this is a minor issue.

Jim