Re: [PATCH v8] This commit adds record full support for rv64gc instruction set

[Andrew Burgess <aburgess@redhat.com>]

Hi Timur,

I think this is looking great now.  I do have a couple of minor follow
on questions/points, but I think this is almost done.  Thanks for all
your work getting this ready for merging.  Notes are inline below...

Timur <timurgol007@gmail.com> writes:

> It includes changes to the following files:
> - gdb/riscv-linux-tdep.c, gdb/riscv-linux-tdep.h: adds facilities to record
> syscalls.
> - gdb/riscv-tdep.c, gdb/riscv-tdep.h: adds facilities to record execution of
> rv64gc instructions.
> - gdb/configure.tgt: adds new files for compilation.
> - gdb/testsuite/lib/gdb.exp: enables testing of full record mode for RISC-V
> targets.
> - gdb/syscalls/riscv-canonicalize-syscall-gen.py: a script to generate
> function that canonicalizes RISC-V syscall. This script can simplify support
> for syscalls on rv32 and rv64 system (currently support only for rv64).  To
> use this script you need to pass a path to a file with syscalls description
> from riscv-glibc (example is in the help message). The script produces a

I guess riscv-glibc is a fork of glibc that is running ahead of the
upstream glibc project?

Is there any reason that this cannot work with the official glibc
project?  That would be preferable I think unless there's a compelling
reason, which ideally we'd document.

I tried running the script using current HEAD of glibc, the only
significant change is:

  diff --git i/gdb/riscv-canonicalize-syscall-gen.c w/gdb/riscv-canonicalize-syscall-gen.c
  index 3749fc32f33..03579984f4a 100644
  --- i/gdb/riscv-canonicalize-syscall-gen.c
  +++ w/gdb/riscv-canonicalize-syscall-gen.c
  @@ -270,7 +270,7 @@ riscv64_canonicalize_syscall (int syscall)
       case 239: return gdb_sys_move_pages;
       /* case 240: return gdb_sys_rt_tgsigqueueinfo;  */
       /* case 241: return gdb_sys_perf_event_open;  */
  -    /* case 242: return gdb_sys_accept4;  */
  +    case 242: return gdb_sys_accept4;
       /* case 243: return gdb_sys_recvmmsg;  */
       /* case 258: return gdb_sys_riscv_hwprobe;  */
       /* case 259: return gdb_sys_riscv_flush_icache;  */

There was also a bunch of new syscalls that are added, but commented
out.  I haven't tried to figure out if the change above is a bad thing
or not though, maybe you could advise.


> diff --git a/gdb/NEWS b/gdb/NEWS
> index 964183910dc..20fdb571047 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -9581,3 +9581,5 @@ GDB now handles cross debugging.  If you are remotely debugging between
>  two different machines, type ``./configure host -target=targ''.
>  Host is the machine where GDB will run; targ is the machine
>  where the program that you are debugging will run.
> +
> + * Add record full support for rv64gc architectures

This entry needs to move to the correct section of the NEWS file.  If
you look for the lines starting '***' you'll see that the first header
is '*** Changes since GDB 16', this is the section where you should
place your entry, probably just before the '* New commands' line.

> diff --git a/gdb/riscv-linux-tdep.h b/gdb/riscv-linux-tdep.h
> new file mode 100644
> index 00000000000..0f481b168d1
> --- /dev/null
> +++ b/gdb/riscv-linux-tdep.h
> @@ -0,0 +1,29 @@
> +/* Copyright (C) 2024-2025 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef RISCV_LINUX_TDEP_H
> +#define RISCV_LINUX_TDEP_H

These include guards need to be renamed to GDB_RISCV_LINUX_TDEP_H.  You
can do this automatically if you like with:

  ./gdb/check-include-guards.py --update gdb/*.h

> +
> +#include "linux-record.h"
> +
> +/* riscv64_canonicalize_syscall maps from the native riscv Linux set
> +   of syscall ids into a canonical set of syscall ids used by
> +   process record.  */
> +
> +extern enum gdb_syscall riscv64_canonicalize_syscall (int syscall);
> +
> +#endif /* RISCV_LINUX_TDEP_H */

> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 410f99e3350..b048ac7078b 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -3718,7 +3718,8 @@ proc supports_reverse {} {
>           || [istarget "i\[34567\]86-*-linux*"]
>           || [istarget "aarch64*-*-linux*"]
>           || [istarget "powerpc*-*-linux*"]
> -         || [istarget "s390*-*-linux*"] } {
> +         || [istarget "s390*-*-linux*"]
> +         || [istarget "riscv*-*-*"] } {

I notice that every other entry in this list restricts the match to
Linux based targets.  Is RISC-V really different here?  Or should RISC-V
be similarly restricted?

Thanks,
Andrew