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

Andrew Burgess
2025-04-10T09:17:00Z

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