RE: [PATCH 10/10] Add minimal and functional NetBSD/amd64 gdbserver

Aktemur, Tankut Baris
2020-09-03T17:42:19Z

On September 2, 2020 7:59 PM, Kamil Rytarowski wrote:
> Implement the following functionality: create_inferior,
> post_create_inferior, attach, kill, detach, mourn, join, thread_alive,
> resume, wait, fetch_registers, store_registers, read_memory, write_memory,
> request_interrupt, supports_read_auxv, read_auxv,
> supports_hardware_single_step, sw_breakpoint_from_kind,
> supports_z_point_type, insert_point, remove_point,
> stopped_by_sw_breakpoint, supports_qxfer_siginfo, qxfer_siginfo,
> supports_stopped_by_sw_breakpoint, supports_non_stop,
> supports_multi_process, supports_fork_events, supports_vfork_events,
> supports_exec_events, supports_disable_randomization,
> supports_qxfer_libraries_svr4, qxfer_libraries_svr4,
> supports_pid_to_exec_file, pid_to_exec_file, thread_name,
> supports_catch_syscall.
>
> The only CPU architecture supported: x86_64.
>
> Implement only support for hardware assisted single step and
> software breakpoint.
>
> Implement support only for regular X86 registers, thus no FPU.
>
> gdb/ChangeLog:

This should be gdbserver/ChangeLog.

>
> * netbsd-low.cc: Add.
> * netbsd-low.h: Likewise.
> * netbsd-x86_64-low.cc: Likewise.
> * Makefile.in (SFILES): Register "netbsd-low.cc", "netbsd-low.h",
> "netbsd-x86_64-low.cc".
> * configure.srv: Add x86_64-*-netbsd*.
> ---
> gdbserver/ChangeLog | 9 +
> gdbserver/Makefile.in | 3 +
> gdbserver/configure.srv | 7 +
> gdbserver/netbsd-low.cc | 1352 ++++++++++++++++++++++++++++++++
> gdbserver/netbsd-low.h | 157 ++++
> gdbserver/netbsd-x86_64-low.cc | 250 ++++++
> 6 files changed, 1778 insertions(+)
> create mode 100644 gdbserver/netbsd-low.cc
> create mode 100644 gdbserver/netbsd-low.h
> create mode 100644 gdbserver/netbsd-x86_64-low.cc
>
> diff --git a/gdbserver/ChangeLog b/gdbserver/ChangeLog
> index e437493b56d..c0732d1a28a 100644
> --- a/gdbserver/ChangeLog
> +++ b/gdbserver/ChangeLog
> @@ -1,3 +1,12 @@
> +2020-08-13 Kamil Rytarowski <n54@gmx.com>
> +
> + * netbsd-low.cc: Add.
> + * netbsd-low.h: Likewise.
> + * netbsd-x86_64-low.cc: Likewise.
> + * Makefile.in (SFILES): Register "netbsd-low.cc", "netbsd-low.h",
> + "netbsd-x86_64-low.cc".
> + * configure.srv: Add x86_64-*-netbsd*.
> +
> 2020-08-13 Simon Marchi <simon.marchi@polymtl.ca>
>
> * server.cc (captured_main): Accept multiple `--selftest=`
> diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in
> index 9d7687be534..1d597b14523 100644
> --- a/gdbserver/Makefile.in
> +++ b/gdbserver/Makefile.in
> @@ -193,6 +193,9 @@ SFILES = \
> $(srcdir)/linux-x86-low.cc \
> $(srcdir)/linux-xtensa-low.cc \
> $(srcdir)/mem-break.cc \
> + $(srcdir)/netbsd-low.cc \
> + $(srcdir)/netbsd-low.h \
> + $(srcdir)/netbsd-x86_64-low.cc \
> $(srcdir)/proc-service.cc \
> $(srcdir)/proc-service.list \
> $(srcdir)/regcache.cc \
> diff --git a/gdbserver/configure.srv b/gdbserver/configure.srv
> index 5e33bd9c54d..12f9d410b7c 100644
> --- a/gdbserver/configure.srv
> +++ b/gdbserver/configure.srv
> @@ -349,6 +349,13 @@ case "${gdbserver_host}" in
> srv_tgtobj="${srv_tgtobj} nat/windows-nat.o"
> srv_tgtobj="${srv_tgtobj} arch/amd64.o arch/i386.o"
> ;;
> + x86_64-*-netbsd*) srv_regobj=""
> + srv_tgtobj="netbsd-low.o netbsd-x86_64-low.o fork-child.o"
> + srv_tgtobj="${srv_tgtobj} nat/fork-inferior.o"
> + srv_tgtobj="${srv_tgtobj} nat/netbsd-nat.o"
> + srv_tgtobj="${srv_tgtobj} arch/amd64.o"
> + srv_netbsd=yes

Is `srv_netbsd` used anywhere?

> + ;;
>
> xtensa*-*-linux*) srv_regobj=reg-xtensa.o
> srv_tgtobj="$srv_linux_obj linux-xtensa-low.o"
> diff --git a/gdbserver/netbsd-low.cc b/gdbserver/netbsd-low.cc
> new file mode 100644
> index 00000000000..db9531c66d3
> --- /dev/null
> +++ b/gdbserver/netbsd-low.cc
> @@ -0,0 +1,1352 @@
> +/* Copyright (C) 2020 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/>. */
> +
> +#include "server.h"
> +#include "target.h"
> +#include "netbsd-low.h"
> +#include "nat/netbsd-nat.h"
> +
> +#include <sys/param.h>
> +#include <sys/types.h>
> +
> +#include <sys/ptrace.h>
> +#include <sys/sysctl.h>
> +
> +#include <limits.h>
> +#include <unistd.h>
> +#include <signal.h>
> +
> +#include <elf.h>
> +
> +#include <type_traits>
> +
> +#include "gdbsupport/eintr.h"
> +#include "gdbsupport/gdb_wait.h"
> +#include "gdbsupport/filestuff.h"
> +#include "gdbsupport/common-inferior.h"
> +#include "nat/fork-inferior.h"
> +#include "hostio.h"
> +
> +int using_threads = 1;
> +
> +const struct target_desc *netbsd_tdesc;
> +
> +/* Call add_process with the given parameters, and initializes

To be consistent with the imperative "Call", "initializes" could read
"initialize".

> + the process' private data. */
> +
> +static struct process_info *
> +netbsd_add_process (int pid, int attached)

There are two uses of this function and in both the return value is ignored.
The function could as well be void.

> +{
> + struct process_info *proc = add_process (pid, attached);
> + proc->tdesc = netbsd_tdesc;
> + proc->priv = nullptr;
> +
> + return proc;
> +}
> +
> +/* Callback used by fork_inferior to start tracing the inferior. */
> +
> +static void
> +netbsd_ptrace_fun ()
> +{
> + /* Switch child to its own process group so that signals won't
> + directly affect GDBserver. */
> + if (setpgid (0, 0) < 0)
> + trace_start_error_with_name (("setpgid"));
> +
> + if (ptrace (PT_TRACE_ME, 0, nullptr, 0) < 0)
> + trace_start_error_with_name (("ptrace"));
> +
> + /* If GDBserver is connected to gdb via stdio, redirect the inferior's
> + stdout to stderr so that inferior i/o doesn't corrupt the connection.
> + Also, redirect stdin to /dev/null. */
> + if (remote_connection_is_stdio ())
> + {
> + if (close (0) < 0)
> + trace_start_error_with_name (("close"));
> + if (open ("/dev/null", O_RDONLY) < 0)
> + trace_start_error_with_name (("open"));
> + if (dup2 (2, 1) < 0)
> + trace_start_error_with_name (("dup2"));
> + if (write (2, "stdin/stdout redirected\n",
> + sizeof ("stdin/stdout redirected\n") - 1) < 0)
> + {
> + /* Errors ignored. */;

Any particular reason for the explicit ';'?

> + }
> + }
> +}
> +
> +/* Register threads in the process specified by PID. */
> +
> +static void
> +netbsd_add_threads (pid_t pid)
> +{
> + auto fn
> + = [] (ptid_t ptid)
> + {
> + add_thread (ptid, nullptr);
> + };
> +
> + netbsd_nat::list_threads (pid, fn);

IMHO, the name "list_threads" gives the impression that it only lists the threads
and is side-effect free. Maybe use something like "for_each_thread", as in gdbthread.h?

> +}
> +
> +/* Implement the create_inferior method of the target_ops vector. */
> +
> +int
> +netbsd_process_target::create_inferior (const char *program,
> + const std::vector<char *> &program_args)
> +{
> + std::string str_program_args = construct_inferior_arguments (program_args);
> +
> + pid_t pid = fork_inferior (program, str_program_args.c_str (),
> + get_environ ()->envp (), netbsd_ptrace_fun,
> + nullptr, nullptr, nullptr, nullptr);
> +
> + netbsd_add_process (pid, 0);
> +
> + post_fork_inferior (pid, program);
> +
> + return pid;
> +}
> +
> +/* Implement the post_create_inferior target_ops method. */
> +
> +void
> +netbsd_process_target::post_create_inferior ()
> +{
> + pid_t pid = current_process ()->pid;
> + netbsd_nat::enable_proc_events (pid);
> +}
> +
> +/* Implement the attach target_ops method. */
> +
> +int
> +netbsd_process_target::attach (unsigned long pid)
> +{
> +
> + if (ptrace (PT_ATTACH, pid, nullptr, 0) != 0)
> + error ("Cannot attach to process %lu: %s (%d)\n", pid,
> + safe_strerror (errno), errno);
> +
> + netbsd_add_process (pid, 1);
> + netbsd_nat::enable_proc_events (pid);
> + netbsd_add_threads (pid);
> +
> + return 0;
> +}
> +
> +/* Returns true if GDB is interested in any child syscalls. */
> +
> +static bool
> +gdb_catching_syscalls_p (pid_t pid)
> +{
> + struct process_info *proc = find_process_pid (pid);
> + return !proc->syscalls_to_catch.empty ();
> +}
> +
> +/* Implement the resume target_ops method. */
> +
> +void
> +netbsd_process_target::resume (struct thread_resume *resume_info, size_t n)
> +{
> + ptid_t resume_ptid = resume_info[0].thread;
> + const int signal = resume_info[0].sig;
> + const bool step = resume_info[0].kind == resume_step;
> +
> + if (resume_ptid == minus_one_ptid)
> + resume_ptid = ptid_of (current_thread);
> +
> + const pid_t pid = resume_ptid.pid ();
> + const lwpid_t lwp = resume_ptid.lwp ();
> + regcache_invalidate_pid (pid);
> +
> + auto fn
> + = [&] (ptid_t ptid)
> + {
> + if (step)
> + {
> + if (ptid.lwp () == lwp || n != 1)
> + {
> + if (ptrace (PT_SETSTEP, pid, NULL, ptid.lwp ()) == -1)
> + perror_with_name (("ptrace"));
> + if (ptrace (PT_RESUME, pid, NULL, ptid.lwp ()) == -1)
> + perror_with_name (("ptrace"));
> + }
> + else
> + {
> + if (ptrace (PT_CLEARSTEP, pid, NULL, ptid.lwp ()) == -1)
> + perror_with_name (("ptrace"));
> + if (ptrace (PT_SUSPEND, pid, NULL, ptid.lwp ()) == -1)
> + perror_with_name (("ptrace"));
> + }
> + }
> + else
> + {
> + if (ptrace (PT_CLEARSTEP, pid, NULL, ptid.lwp ()) == -1)
> + perror_with_name (("ptrace"));
> + if (ptrace (PT_RESUME, pid, NULL, ptid.lwp ()) == -1)
> + perror_with_name (("ptrace"));
> + }
> + };
> +
> + netbsd_nat::list_threads (pid, fn);
> +
> + int request = gdb_catching_syscalls_p (pid) ? PT_CONTINUE : PT_SYSCALL;
> +
> + errno = 0;
> + ptrace (request, pid, (void *)1, signal);
> + if (errno)
> + perror_with_name (("ptrace"));
> +}
> +
> +/* Returns true if GDB is interested in the reported SYSNO syscall. */
> +
> +static bool
> +netbsd_catch_this_syscall (int sysno)
> +{
> + struct process_info *proc = current_process ();
> +
> + if (proc->syscalls_to_catch.empty ())
> + return false;
> +
> + if (proc->syscalls_to_catch[0] == ANY_SYSCALL)
> + return true;
> +
> + for (int iter : proc->syscalls_to_catch)
> + if (iter == sysno)
> + return true;
> +
> + return false;
> +}
> +
> +/* Helper function for child_wait and the derivatives of child_wait.
> + HOSTSTATUS is the waitstatus from wait() or the equivalent; store our
> + translation of that in OURSTATUS. */

A blank line here between the comment and the function header.

> +static void
> +netbsd_store_waitstatus (struct target_waitstatus *ourstatus, int hoststatus)
> +{
> + if (WIFEXITED (hoststatus))
> + {
> + ourstatus->kind = TARGET_WAITKIND_EXITED;
> + ourstatus->value.integer = WEXITSTATUS (hoststatus);
> + }
> + else if (!WIFSTOPPED (hoststatus))
> + {
> + ourstatus->kind = TARGET_WAITKIND_SIGNALLED;
> + ourstatus->value.sig = gdb_signal_from_host (WTERMSIG (hoststatus));
> + }
> + else
> + {
> + ourstatus->kind = TARGET_WAITKIND_STOPPED;
> + ourstatus->value.sig = gdb_signal_from_host (WSTOPSIG (hoststatus));
> + }
> +}
> +
> +/* Implement a safe wrapper around waitpid(). */
> +
> +static pid_t
> +netbsd_waitpid (ptid_t ptid, struct target_waitstatus *ourstatus, int options)
> +{
> + int status;
> +
> + pid_t pid = gdb::handle_eintr (::waitpid, ptid.pid (), &status, options);
> +
> + if (pid == -1)
> + perror_with_name (_("Child process unexpectedly missing"));
> +
> + netbsd_store_waitstatus (ourstatus, status);
> + return pid;
> +}
> +
> +
> +/* Implement the wait target_ops method.
> +
> + Wait for the child specified by PTID to do something. Return the
> + process ID of the child, or MINUS_ONE_PTID in case of error; store
> + the status in *OURSTATUS. */
> +
> +static ptid_t
> +netbsd_wait (ptid_t ptid, struct target_waitstatus *ourstatus,
> + int target_options)
> +{
> + pid_t pid = netbsd_waitpid (ptid, ourstatus, target_options);
> + ptid_t wptid = ptid_t (pid);
> +
> + if (pid == 0)
> + {
> + gdb_assert (target_options & TARGET_WNOHANG);
> + ourstatus->kind = TARGET_WAITKIND_IGNORE;
> + return null_ptid;
> + }
> +
> + gdb_assert (pid != -1);
> +
> + /* If the child stopped, keep investigating its status. */
> + if (ourstatus->kind != TARGET_WAITKIND_STOPPED)
> + return wptid;
> +
> + /* Extract the event and thread that received a signal. */
> + ptrace_siginfo_t psi;
> + if (ptrace (PT_GET_SIGINFO, pid, &psi, sizeof (psi)) == -1)
> + perror_with_name (("ptrace"));
> +
> + /* Pick child's siginfo_t. */
> + siginfo_t *si = &psi.psi_siginfo;
> +
> + lwpid_t lwp = psi.psi_lwpid;
> +
> + int signo = si->si_signo;
> + const int code = si->si_code;
> +
> + /* Construct PTID with a specified thread that received the event.
> + If a signal was targeted to the whole process, lwp is 0. */
> + wptid = ptid_t (pid, lwp, 0);
> +
> + /* Bail out on non-debugger oriented signals.. */

Double periods.

> + if (signo != SIGTRAP)
> + return wptid;
> +
> + /* Stop examining non-debugger oriented SIGTRAP codes. */
> + if (code <= SI_USER || code == SI_NOINFO)
> + return wptid;
> +
> + /* Process state for threading events */

End with dot-space-space?

> + ptrace_state_t pst = {};
> + if (code == TRAP_LWP)
> + {
> + if (ptrace (PT_GET_PROCESS_STATE, pid, &pst, sizeof (pst)) == -1)
> + perror_with_name (("ptrace"));
> + }

I think braces can be removed.

> +
> + if (code == TRAP_LWP && pst.pe_report_event == PTRACE_LWP_EXIT)
> + {
> + /* If GDB attaches to a multi-threaded process, exiting
> + threads might be skipped during post_attach that
> + have not yet reported their PTRACE_LWP_EXIT event.
> + Ignore exited events for an unknown LWP. */
> + thread_info *thr = find_thread_ptid (wptid);
> + if (thr == nullptr)
> + ourstatus->kind = TARGET_WAITKIND_SPURIOUS;
> + else
> + {
> + ourstatus->kind = TARGET_WAITKIND_THREAD_EXITED;
> + /* NetBSD does not store an LWP exit status. */
> + ourstatus->value.integer = 0;
> +
> + remove_thread (thr);
> + }
> + return wptid;
> + }
> +
> + if (find_thread_ptid (ptid_t (pid)))
> + current_thread = find_thread_ptid (wptid);
> +
> + if (code == TRAP_LWP && pst.pe_report_event == PTRACE_LWP_CREATE)
> + {
> + /* If GDB attaches to a multi-threaded process, newborn
> + threads might be added by nbsd_add_threads that have
> + not yet reported their PTRACE_LWP_CREATE event. Ignore
> + born events for an already-known LWP. */
> + if (find_thread_ptid (wptid))
> + ourstatus->kind = TARGET_WAITKIND_SPURIOUS;
> + else
> + {
> + add_thread (wptid, NULL);
> + ourstatus->kind = TARGET_WAITKIND_THREAD_CREATED;
> + }
> + return wptid;
> + }
> +
> + if (code == TRAP_EXEC)
> + {
> + ourstatus->kind = TARGET_WAITKIND_EXECD;
> + ourstatus->value.execd_pathname
> + = xstrdup (netbsd_nat::pid_to_exec_file (pid));
> + return wptid;
> + }
> +
> + if (code == TRAP_TRACE)
> + {
> + return wptid;
> + }

Braces can be removed.

> +
> + if (code == TRAP_SCE || code == TRAP_SCX)
> + {
> + int sysnum = si->si_sysnum;
> +
> + if (!netbsd_catch_this_syscall(sysnum))
> + {
> + /* If the core isn't interested in this event, ignore it. */
> + ourstatus->kind = TARGET_WAITKIND_SPURIOUS;
> + return wptid;
> + }
> +
> + ourstatus->kind =

The assignment operator should be on the next line.

> + (code == TRAP_SCE) ? TARGET_WAITKIND_SYSCALL_ENTRY :
> + TARGET_WAITKIND_SYSCALL_RETURN;
> + ourstatus->value.syscall_number = sysnum;
> + return wptid;
> + }
> +
> + if (code == TRAP_BRKPT)
> + {
> +#ifdef PTRACE_BREAKPOINT_ADJ
> + CORE_ADDR pc;
> + struct reg r;
> + ptrace (PT_GETREGS, pid, &r, psi.psi_lwpid);
> + pc = PTRACE_REG_PC (&r);
> + PTRACE_REG_SET_PC (&r, pc - PTRACE_BREAKPOINT_ADJ);
> + ptrace (PT_SETREGS, pid, &r, psi.psi_lwpid);
> +#endif
> + return wptid;
> + }
> +
> + /* Unclassified SIGTRAP event. */
> + ourstatus->kind = TARGET_WAITKIND_SPURIOUS;
> + return wptid;
> +}
> +
> +/* Implement the wait target_ops method. */
> +
> +ptid_t
> +netbsd_process_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
> + int target_options)
> +{
> + while (true)
> + {
> + ptid_t wptid = netbsd_wait (ptid, ourstatus, target_options);
> +
> + /* Register thread in the gdbcore if a thread was not reported earlier.
> + This is required after ::create_inferior, when the gdbcore does not
> + know about the first internal thread.
> + This may also happen on attach, when an event is registered on a thread
> + that was not fully initialized during the attach stage. */

Dot-space-space.

> + if (wptid.lwp () != 0 && !find_thread_ptid (wptid)
> + && ourstatus->kind != TARGET_WAITKIND_THREAD_EXITED)
> + add_thread (wptid, nullptr);
> +
> + switch (ourstatus->kind)
> + {
> + case TARGET_WAITKIND_EXITED:
> + case TARGET_WAITKIND_STOPPED:
> + case TARGET_WAITKIND_SIGNALLED:
> + case TARGET_WAITKIND_FORKED:
> + case TARGET_WAITKIND_VFORKED:
> + case TARGET_WAITKIND_EXECD:
> + case TARGET_WAITKIND_VFORK_DONE:
> + case TARGET_WAITKIND_SYSCALL_ENTRY:
> + case TARGET_WAITKIND_SYSCALL_RETURN:
> + /* Pass the result to the generic code. */
> + return wptid;
> + case TARGET_WAITKIND_THREAD_CREATED:
> + case TARGET_WAITKIND_THREAD_EXITED:
> + /* The core needlessly stops on these events. */
> + /* FALLTHROUGH */
> + case TARGET_WAITKIND_SPURIOUS:
> + /* Spurious events are unhandled by the gdbserver core. */
> + if (ptrace (PT_CONTINUE, current_process ()->pid, (void *) 1, 0)
> + == -1)
> + perror_with_name (("ptrace"));
> + break;
> + default:
> + error (("Unknwon stopped status"));

Typo "Unknwon".

> + }
> + }
> +}
> +
> +/* Implement the kill target_ops method. */
> +
> +int
> +netbsd_process_target::kill (process_info *process)
> +{
> + pid_t pid = process->pid;
> + ptrace (PT_KILL, pid, nullptr, 0);
> +
> + int status;
> + gdb::handle_eintr (::waitpid, pid, &status, 0);

The "kill" target op is supposed to return -1 on failure.
So, maybe not ignore the return value of handle_eintr here?

> + mourn (process);
> + return 0;
> +}
> +
> +/* Implement the detach target_ops method. */
> +
> +int
> +netbsd_process_target::detach (process_info *process)
> +{
> + pid_t pid = process->pid;
> +
> + ptrace (PT_DETACH, pid, (void *) 1, 0);
> + mourn (process);
> + return 0;
> +}
> +
> +/* Implement the mourn target_ops method. */
> +
> +void
> +netbsd_process_target::mourn (struct process_info *proc)
> +{
> + for_each_thread (proc->pid, remove_thread);
> +
> + remove_process (proc);
> +}
> +
> +/* Implement the join target_ops method. */
> +
> +void
> +netbsd_process_target::join (int pid)
> +{
> + /* The PT_DETACH is sufficient to detach from the process.
> + So no need to do anything extra. */
> +}
> +
> +/* Implement the thread_alive target_ops method. */
> +
> +bool
> +netbsd_process_target::thread_alive (ptid_t ptid)
> +{
> + return netbsd_nat::thread_alive (ptid);
> +}
> +
> +/* Implement the fetch_registers target_ops method. */
> +
> +void
> +netbsd_process_target::fetch_registers (struct regcache *regcache, int regno)
> +{
> + struct netbsd_regset_info *regset = netbsd_target_regsets;
> + ptid_t inferior_ptid = ptid_of (current_thread);
> +
> + while (regset->size >= 0)
> + {
> + char *buf = (char *) xmalloc (regset->size);
> + int res = ptrace (regset->get_request, inferior_ptid.pid (), buf,
> + inferior_ptid.lwp ());
> + if (res == -1)
> + perror_with_name (("ptrace"));
> + regset->store_function (regcache, buf);
> + free (buf);
> + regset++;
> + }
> +}
> +
> +/* Implement the store_registers target_ops method. */
> +
> +void
> +netbsd_process_target::store_registers (struct regcache *regcache, int regno)
> +{
> + struct netbsd_regset_info *regset = netbsd_target_regsets;
> + ptid_t inferior_ptid = ptid_of (current_thread);
> +
> + while (regset->size >= 0)
> + {
> + char *buf = (char *)xmalloc (regset->size);
> + int res = ptrace (regset->get_request, inferior_ptid.pid (), buf,
> + inferior_ptid.lwp ());
> + if (res == -1)
> + perror_with_name (("ptrace"));
> + if (res == 0)
> + {
> + /* Then overlay our cached registers on that. */
> + regset->fill_function (regcache, buf);
> + /* Only now do we write the register set. */
> + res = ptrace (regset->set_request, inferior_ptid.pid (), buf,
> + inferior_ptid.lwp ());
> + }
> + free (buf);
> + regset++;
> + }
> +}
> +
> +/* Implement the read_memory target_ops method. */
> +
> +int
> +netbsd_process_target::read_memory (CORE_ADDR memaddr, unsigned char *myaddr,
> + int size)
> +{
> + struct ptrace_io_desc io;
> + io.piod_op = PIOD_READ_D;
> + io.piod_len = size;
> +
> + pid_t pid = current_process ()->pid;
> +
> + int bytes_read = 0;
> +
> + if (size == 0)
> + {
> + /* Zero length write always succeeds. */
> + return 0;
> + }
> + do
> + {
> + io.piod_offs = (void *)(memaddr + bytes_read);
> + io.piod_addr = myaddr + bytes_read;
> +
> + int rv = ptrace (PT_IO, pid, &io, 0);
> + if (rv == -1)
> + return errno;
> + if (io.piod_len == 0)
> + return 0;
> +
> + bytes_read += io.piod_len;
> + io.piod_len = size - bytes_read;
> + }
> + while (bytes_read < size);
> +
> + return 0;
> +}
> +
> +/* Implement the write_memory target_ops method. */
> +
> +int
> +netbsd_process_target::write_memory (CORE_ADDR memaddr,
> + const unsigned char *myaddr, int size)
> +{
> + struct ptrace_io_desc io;
> + io.piod_op = PIOD_WRITE_D;
> + io.piod_len = size;
> +
> + pid_t pid = current_process ()->pid;
> +
> + int bytes_written = 0;
> +
> + if (size == 0)
> + {
> + /* Zero length write always succeeds. */
> + return 0;
> + }
> +
> + do
> + {
> + io.piod_addr = (void *)(myaddr + bytes_written);
> + io.piod_offs = (void *)(memaddr + bytes_written);
> +
> + int rv = ptrace (PT_IO, pid, &io, 0);
> + if (rv == -1)
> + return errno;
> + if (io.piod_len == 0)
> + return 0;
> +
> + bytes_written += io.piod_len;
> + io.piod_len = size - bytes_written;
> + }
> + while (bytes_written < size);
> +
> + return 0;
> +}
> +
> +/* Implement the kill_request target_ops method. */

"kill_request" -> "request_interrupt".

> +
> +void
> +netbsd_process_target::request_interrupt ()
> +{
> + ptid_t inferior_ptid = ptid_of (get_first_thread ());
> +
> + ::kill (inferior_ptid.pid(), SIGINT);
> +}
> +

I think a function comment is needed here.

> +static size_t
> +netbsd_read_auxv(pid_t pid, void *offs, void *addr, size_t len)
> +{
> + struct ptrace_io_desc pio;
> +
> + pio.piod_op = PIOD_READ_AUXV;
> + pio.piod_offs = offs;
> + pio.piod_addr = addr;
> + pio.piod_len = len;
> +
> + if (ptrace (PT_IO, pid, &pio, 0) == -1)
> + perror_with_name (("ptrace"));
> +
> + return pio.piod_len;
> +}
> +
> +/* Copy LEN bytes from inferior's auxiliary vector starting at OFFSET
> + to debugger memory starting at MYADDR. */
> +
> +int
> +netbsd_process_target::read_auxv (CORE_ADDR offset,
> + unsigned char *myaddr, unsigned int len)
> +{
> + pid_t pid = pid_of (current_thread);
> +
> + return netbsd_read_auxv (pid, (void *) (intptr_t) offset, myaddr, len);
> +}
> +
> +bool
> +netbsd_process_target::supports_z_point_type (char z_type)
> +{
> + switch (z_type)
> + {
> + case Z_PACKET_SW_BP:
> + return true;
> + case Z_PACKET_HW_BP:
> + case Z_PACKET_WRITE_WP:
> + case Z_PACKET_READ_WP:
> + case Z_PACKET_ACCESS_WP:
> + default:
> + return false; /* Not supported. */
> + }
> +}
> +
> +/* Insert {break/watch}point at address ADDR. SIZE is not used. */
> +
> +int
> +netbsd_process_target::insert_point (enum raw_bkpt_type type, CORE_ADDR addr,
> + int size, struct raw_breakpoint *bp)
> +{
> + switch (type)
> + {
> + case raw_bkpt_type_sw:
> + return insert_memory_breakpoint (bp);
> + case raw_bkpt_type_hw:
> + case raw_bkpt_type_write_wp:
> + case raw_bkpt_type_read_wp:
> + case raw_bkpt_type_access_wp:
> + default:
> + return 1; /* Not supported. */
> + }
> +}
> +
> +/* Remove {break/watch}point at address ADDR. SIZE is not used. */
> +
> +int
> +netbsd_process_target::remove_point (enum raw_bkpt_type type, CORE_ADDR addr,
> + int size, struct raw_breakpoint *bp)
> +{
> + switch (type)
> + {
> + case raw_bkpt_type_sw:
> + return remove_memory_breakpoint (bp);
> + case raw_bkpt_type_hw:
> + case raw_bkpt_type_write_wp:
> + case raw_bkpt_type_read_wp:
> + case raw_bkpt_type_access_wp:
> + default:
> + return 1; /* Not supported. */
> + }
> +}
> +
> +/* Implement the to_stopped_by_sw_breakpoint target_ops
> + method. */

"to_stopped_by_sw_breakpoint" -> "stopped_by_sw_breakpoint".

> +
> +bool
> +netbsd_process_target::stopped_by_sw_breakpoint ()
> +{
> + ptrace_siginfo_t psi;
> + pid_t pid = current_process ()->pid;
> +
> + if (ptrace (PT_GET_SIGINFO, pid, &psi, sizeof (psi)) == -1)
> + perror_with_name (("ptrace"));
> +
> + return psi.psi_siginfo.si_signo == SIGTRAP &&
> + psi.psi_siginfo.si_code == TRAP_BRKPT;
> +}
> +
> +/* Implement the to_supports_stopped_by_sw_breakpoint target_ops
> + method. */

The "to_" prefix shall be removed here, too.

> +
> +bool
> +netbsd_process_target::supports_stopped_by_sw_breakpoint ()
> +{
> + return true;
> +}
> +
> +/* Implement the supports_qxfer_siginfo target_ops method. */
> +
> +bool
> +netbsd_process_target::supports_qxfer_siginfo ()
> +{
> + return true;
> +}
> +
> +/* Implement the qxfer_siginfo target_ops method. */
> +
> +int
> +netbsd_process_target::qxfer_siginfo (const char *annex, unsigned char *readbuf,
> + unsigned const char *writebuf,
> + CORE_ADDR offset, int len)
> +{
> + if (current_thread == nullptr)
> + return -1;
> +
> + pid_t pid = current_process ()->pid;
> +
> + return netbsd_nat::qxfer_siginfo(pid, annex, readbuf, writebuf, offset, len);
> +}
> +
> +/* Implement the supports_non_stop target_ops method. */
> +
> +bool
> +netbsd_process_target::supports_non_stop ()
> +{
> + return false;
> +}
> +
> +/* Implement the supports_multi_process target_ops method. */
> +
> +bool
> +netbsd_process_target::supports_multi_process ()
> +{
> + return true;
> +}
> +
> +/* Check if fork events are supported. */
> +
> +bool
> +netbsd_process_target::supports_fork_events ()
> +{
> + return false;
> +}
> +
> +/* Check if vfork events are supported. */
> +
> +bool
> +netbsd_process_target::supports_vfork_events ()
> +{
> + return false;
> +}
> +
> +/* Check if exec events are supported. */
> +
> +bool
> +netbsd_process_target::supports_exec_events ()
> +{
> + return true;
> +}
> +
> +/* Implement the supports_disable_randomization target_ops method. */
> +
> +bool
> +netbsd_process_target::supports_disable_randomization ()
> +{
> + return false;
> +}

Some of the target ops above do not change the base class behavior. The override
can be omitted for code simplicity. I think it's also OK to retain them if you want
to make the definitions explicit.

> +
> +/* Extract &phdr and num_phdr in the inferior. Return 0 on success. */
> +
> +template <typename T>
> +int get_phdr_phnum_from_proc_auxv (const pid_t pid,
> + CORE_ADDR *phdr_memaddr, int *num_phdr)
> +{
> + typedef typename std::conditional<sizeof(T) == sizeof(int64_t),
> + Aux64Info, Aux32Info>::type auxv_type;
> + const size_t auxv_size = sizeof (auxv_type);
> + const size_t auxv_buf_size = 128 * sizeof (auxv_type);
> +
> + char *auxv_buf = (char *) xmalloc (auxv_buf_size);
> +
> + netbsd_read_auxv (pid, nullptr, auxv_buf, auxv_buf_size);
> +
> + *phdr_memaddr = 0;
> + *num_phdr = 0;
> +
> + for (char *buf = auxv_buf; buf < (auxv_buf + auxv_buf_size); buf += auxv_size)
> + {
> + auxv_type *const aux = (auxv_type *) buf;
> +
> + switch (aux->a_type)
> + {
> + case AT_PHDR:
> + *phdr_memaddr = aux->a_v;
> + break;
> + case AT_PHNUM:
> + *num_phdr = aux->a_v;
> + break;
> + }
> +
> + if (*phdr_memaddr != 0 && *num_phdr != 0)
> + break;
> + }
> +
> + xfree (auxv_buf);
> +
> + if (*phdr_memaddr == 0 || *num_phdr == 0)
> + {
> + warning ("Unexpected missing AT_PHDR and/or AT_PHNUM: "
> + "phdr_memaddr = %ld, phdr_num = %d",
> + (long) *phdr_memaddr, *num_phdr);

It might be better to use `core_addr_to_string` or `core_addr_to_string_nz`
to print phdr_memaddr.

> + return 2;
> + }
> +
> + return 0;
> +}
> +
> +/* Return &_DYNAMIC (via PT_DYNAMIC) in the inferior, or 0 if not present. */
> +
> +template <typename T>
> +static CORE_ADDR
> +get_dynamic (netbsd_process_target *target, const pid_t pid)
> +{
> + typedef typename std::conditional<sizeof(T) == sizeof(int64_t),
> + Elf64_Phdr, Elf32_Phdr>::type phdr_type;
> + const int phdr_size = sizeof (phdr_type);
> +
> + CORE_ADDR phdr_memaddr;
> + int num_phdr;
> + if (get_phdr_phnum_from_proc_auxv<T> (pid, &phdr_memaddr, &num_phdr))
> + return 0;
> +
> + std::vector<unsigned char> phdr_buf;
> + phdr_buf.resize (num_phdr * phdr_size);
> +
> + if ((*target).read_memory (phdr_memaddr, phdr_buf.data (), phdr_buf.size ()))

Why not simply `target->read_memory`?
Also, it might be better to explicitly check != 0.

> + return 0;
> +
> + /* Compute relocation: it is expected to be 0 for "regular" executables,
> + non-zero for PIE ones. */
> + CORE_ADDR relocation = -1;
> + for (int i = 0; relocation == -1 && i < num_phdr; i++)
> + {
> + phdr_type *const p = (phdr_type *) (phdr_buf.data() + i * phdr_size);
> +
> + if (p->p_type == PT_PHDR)
> + relocation = phdr_memaddr - p->p_vaddr;
> + }
> +
> + if (relocation == -1)
> + {
> + /* PT_PHDR is optional, but necessary for PIE in general. Fortunately
> + any real world executables, including PIE executables, have always
> + PT_PHDR present. PT_PHDR is not present in some shared libraries or
> + in fpc (Free Pascal 2.4) binaries but neither of those have a need for
> + or present DT_DEBUG anyway (fpc binaries are statically linked).
> +
> + Therefore if there exists DT_DEBUG there is always also PT_PHDR.
> +
> + GDB could find RELOCATION also from AT_ENTRY - e_entry. */
> +
> + return 0;
> + }
> +
> + for (int i = 0; i < num_phdr; i++)
> + {
> + phdr_type *const p = (phdr_type *) (phdr_buf.data () + i * phdr_size);
> +
> + if (p->p_type == PT_DYNAMIC)
> + return p->p_vaddr + relocation;
> + }
> +
> + return 0;
> +}
> +
> +/* Return &_r_debug in the inferior, or -1 if not present. Return value
> + can be 0 if the inferior does not yet have the library list initialized.
> + We look for DT_MIPS_RLD_MAP first. MIPS executables use this instead of
> + DT_DEBUG, although they sometimes contain an unused DT_DEBUG entry too. */
> +
> +template <typename T>
> +static CORE_ADDR
> +get_r_debug (netbsd_process_target *target, const int pid)
> +{
> + typedef typename std::conditional<sizeof(T) == sizeof(int64_t),
> + Elf64_Dyn, Elf32_Dyn>::type dyn_type;
> + const int dyn_size = sizeof (dyn_type);
> + unsigned char buf[sizeof (dyn_type)]; /* The larger of the two. */
> + CORE_ADDR map = -1;
> +
> + CORE_ADDR dynamic_memaddr = get_dynamic<T> (target, pid);
> + if (dynamic_memaddr == 0)
> + return map;
> +
> + while ((*target).read_memory (dynamic_memaddr, buf, dyn_size) == 0)

The `target->read_memory` comment here, too.

> + {
> + dyn_type *const dyn = (dyn_type *) buf;
> +#if defined DT_MIPS_RLD_MAP
> + union
> + {
> + T map;
> + unsigned char buf[sizeof (T)];
> + }
> + rld_map;
> +
> + if (dyn->d_tag == DT_MIPS_RLD_MAP)
> + {
> + if (read_memory (dyn->d_un.d_val,
> + rld_map.buf, sizeof (rld_map.buf)) == 0)
> + return rld_map.map;
> + else
> + break;
> + }
> +#endif /* DT_MIPS_RLD_MAP */
> +
> + if (dyn->d_tag == DT_DEBUG && map == -1)
> + map = dyn->d_un.d_val;
> +
> + if (dyn->d_tag == DT_NULL)
> + break;
> +
> + dynamic_memaddr += dyn_size;
> + }
> +
> + return map;
> +}
> +
> +/* Read one pointer from MEMADDR in the inferior. */
> +
> +static int
> +read_one_ptr (netbsd_process_target *target, CORE_ADDR memaddr, CORE_ADDR *ptr,
> + int ptr_size)
> +{
> + /* Go through a union so this works on either big or little endian
> + hosts, when the inferior's pointer size is smaller than the size
> + of CORE_ADDR. It is assumed the inferior's endianness is the
> + same of the superior's. */
> +
> + union
> + {
> + CORE_ADDR core_addr;
> + unsigned int ui;
> + unsigned char uc;
> + } addr;
> +
> + int ret = (*target).read_memory (memaddr, &addr.uc, ptr_size);

Here, too.

> + if (ret == 0)
> + {
> + if (ptr_size == sizeof (CORE_ADDR))
> + *ptr = addr.core_addr;
> + else if (ptr_size == sizeof (unsigned int))
> + *ptr = addr.ui;
> + else
> + gdb_assert_not_reached ("unhandled pointer size");
> + }
> + return ret;
> +}
> +
> +/* Construct qXfer:libraries-svr4:read reply. */
> +
> +template <typename T>
> +int
> +netbsd_qxfer_libraries_svr4 (netbsd_process_target *target,
> + const pid_t pid, const char *annex,
> + unsigned char *readbuf,
> + unsigned const char *writebuf,
> + CORE_ADDR offset, int len)
> +{
> + struct link_map_offsets
> + {
> + /* Offset and size of r_debug.r_version. */
> + int r_version_offset;
> +
> + /* Offset and size of r_debug.r_map. */
> + int r_map_offset;
> +
> + /* Offset to l_addr field in struct link_map. */
> + int l_addr_offset;
> +
> + /* Offset to l_name field in struct link_map. */
> + int l_name_offset;
> +
> + /* Offset to l_ld field in struct link_map. */
> + int l_ld_offset;
> +
> + /* Offset to l_next field in struct link_map. */
> + int l_next_offset;
> +
> + /* Offset to l_prev field in struct link_map. */
> + int l_prev_offset;
> + };
> +
> + static const struct link_map_offsets lmo_32bit_offsets =
> + {
> + 0, /* r_version offset. */
> + 4, /* r_debug.r_map offset. */
> + 0, /* l_addr offset in link_map. */
> + 4, /* l_name offset in link_map. */
> + 8, /* l_ld offset in link_map. */
> + 12, /* l_next offset in link_map. */
> + 16 /* l_prev offset in link_map. */
> + };
> +
> + static const struct link_map_offsets lmo_64bit_offsets =
> + {
> + 0, /* r_version offset. */
> + 8, /* r_debug.r_map offset. */
> + 0, /* l_addr offset in link_map. */
> + 8, /* l_name offset in link_map. */
> + 16, /* l_ld offset in link_map. */
> + 24, /* l_next offset in link_map. */
> + 32 /* l_prev offset in link_map. */
> + };
> +
> + CORE_ADDR lm_addr = 0, lm_prev = 0;
> + CORE_ADDR l_name, l_addr, l_ld, l_next, l_prev;
> + int header_done = 0;
> +
> + const struct link_map_offsets *lmo
> + = (sizeof (T) == sizeof (int64_t))
> + ? &lmo_64bit_offsets : &lmo_32bit_offsets;

An extra pair of parens can be used to better align '?'. Like this:

const struct link_map_offsets *lmo
= ((sizeof (T) == sizeof (int64_t))
? &lmo_64bit_offsets : &lmo_32bit_offsets);

> + int ptr_size = sizeof (T);
> +
> + while (annex[0] != '\0')
> + {
> + const char *sep = strchr (annex, '=');
> + if (sep == nullptr)
> + break;
> +
> + int name_len = sep - annex;
> + CORE_ADDR *addrp;
> + if (name_len == 5 && startswith (annex, "start"))
> + addrp = &lm_addr;
> + else if (name_len == 4 && startswith (annex, "prev"))
> + addrp = &lm_prev;
> + else
> + {
> + annex = strchr (sep, ';');
> + if (annex == nullptr)
> + break;
> + annex++;
> + continue;
> + }
> +
> + annex = decode_address_to_semicolon (addrp, sep + 1);
> + }
> +
> + if (lm_addr == 0)
> + {
> + CORE_ADDR r_debug = get_r_debug<T> (target, pid);
> +
> + /* We failed to find DT_DEBUG. Such situation will not change
> + for this inferior - do not retry it. Report it to GDB as
> + E01, see for the reasons at the GDB solib-svr4.c side. */
> + if (r_debug == (CORE_ADDR) -1)
> + return -1;
> +
> + if (r_debug != 0)
> + {
> + if (read_one_ptr (target, r_debug + lmo->r_map_offset,
> + &lm_addr, ptr_size) != 0)
> + {
> + warning ("unable to read r_map from 0x%lx",
> + (long) r_debug + lmo->r_map_offset);

Again, `core_addr_to_string` could be used.

> + }
> + }

Braces can be removed in both conditional statements above. And, if you prefer,
the conditions can be merged via &&.

> + }
> +
> + std::string document = "<library-list-svr4 version=\"1.0\"";
> +
> + while (lm_addr
> + && read_one_ptr (target, lm_addr + lmo->l_name_offset,
> + &l_name, ptr_size) == 0
> + && read_one_ptr (target, lm_addr + lmo->l_addr_offset,
> + &l_addr, ptr_size) == 0
> + && read_one_ptr (target, lm_addr + lmo->l_ld_offset,
> + &l_ld, ptr_size) == 0
> + && read_one_ptr (target, lm_addr + lmo->l_prev_offset,
> + &l_prev, ptr_size) == 0
> + && read_one_ptr (target, lm_addr + lmo->l_next_offset,
> + &l_next, ptr_size) == 0)
> + {
> + if (lm_prev != l_prev)
> + {
> + warning ("Corrupted shared library list: 0x%lx != 0x%lx",
> + (long) lm_prev, (long) l_prev);
> + break;
> + }
> +
> + /* Ignore the first entry even if it has valid name as the first entry
> + corresponds to the main executable. The first entry should not be
> + skipped if the dynamic loader was loaded late by a static executable
> + (see solib-svr4.c parameter ignore_first). But in such case the main
> + executable does not have PT_DYNAMIC present and this function already
> + exited above due to failed get_r_debug. */
> + if (lm_prev == 0)
> + string_appendf (document, " main-lm=\"0x%lx\"",
> + (unsigned long) lm_addr);
> + else
> + {
> + unsigned char libname[PATH_MAX];
> +
> + /* Not checking for error because reading may stop before
> + we've got PATH_MAX worth of characters. */
> + libname[0] = '\0';
> + (*target).read_memory (l_name, libname, sizeof (libname) - 1);
> + libname[sizeof (libname) - 1] = '\0';
> + if (libname[0] != '\0')
> + {
> + if (!header_done)
> + {
> + /* Terminate `<library-list-svr4'. */
> + document += '>';
> + header_done = 1;
> + }
> +
> + string_appendf (document, "<library name=\"");
> + xml_escape_text_append (&document, (char *) libname);
> + string_appendf (document, "\" lm=\"0x%lx\" "
> + "l_addr=\"0x%lx\" l_ld=\"0x%lx\"/>",
> + (unsigned long) lm_addr, (unsigned long) l_addr,
> + (unsigned long) l_ld);
> + }
> + }
> +
> + lm_prev = lm_addr;
> + lm_addr = l_next;
> + }
> +
> + if (!header_done)
> + {
> + /* Empty list; terminate `<library-list-svr4'. */
> + document += "/>";
> + }
> + else
> + document += "</library-list-svr4>";
> +
> + int document_len = document.length ();
> + if (offset < document_len)
> + document_len -= offset;
> + else
> + document_len = 0;
> + if (len > document_len)
> + len = document_len;
> +
> + memcpy (readbuf, document.data () + offset, len);
> +
> + return len;
> +}
> +
> +/* Return true if FILE is a 64-bit ELF file,
> + false if the file is not a 64-bit ELF file,
> + and error if the file is not accessible or doesn't exist. */
> +
> +static bool
> +elf_64_file_p (const char *file)
> +{
> + int fd = gdb::handle_eintr (::open, file, O_RDONLY);
> + if (fd < 0)
> + perror_with_name (("open"));
> +
> + Elf64_Ehdr header;
> + ssize_t ret = gdb::handle_eintr (::read, fd, &header, sizeof (header));
> + if (ret == -1)
> + perror_with_name (("read"));
> + gdb::handle_eintr (::close, fd);
> + if (ret != sizeof (header))
> + error ("Cannot read ELF file header: %s", file);
> +
> + if (header.e_ident[EI_MAG0] != ELFMAG0
> + || header.e_ident[EI_MAG1] != ELFMAG1
> + || header.e_ident[EI_MAG2] != ELFMAG2
> + || header.e_ident[EI_MAG3] != ELFMAG3)
> + error ("Unrecognized ELF file header: %s", file);
> +
> + return header.e_ident[EI_CLASS] == ELFCLASS64;
> +}
> +
> +/* Construct qXfer:libraries-svr4:read reply. */
> +
> +int
> +netbsd_process_target::qxfer_libraries_svr4 (const char *annex,
> + unsigned char *readbuf,
> + unsigned const char *writebuf,
> + CORE_ADDR offset, int len)
> +{
> + if (writebuf != nullptr)
> + return -2;
> + if (readbuf == nullptr)
> + return -1;
> +
> + struct process_info *proc = current_process ();
> + pid_t pid = proc->pid;
> + bool is_elf64 = elf_64_file_p (netbsd_nat::pid_to_exec_file (pid));
> +
> + if (is_elf64)
> + return netbsd_qxfer_libraries_svr4<int64_t> (this, pid, annex, readbuf,
> + writebuf, offset, len);
> + else
> + return netbsd_qxfer_libraries_svr4<int32_t> (this, pid, annex, readbuf,
> + writebuf, offset, len);
> +}
> +
> +/* Implement the supports_qxfer_libraries_svr4 target_ops method. */
> +
> +bool
> +netbsd_process_target::supports_qxfer_libraries_svr4 ()
> +{
> + return true;
> +}
> +
> +/* Return the name of a file that can be opened to get the symbols for
> + the child process identified by PID. */
> +
> +char *
> +netbsd_process_target::pid_to_exec_file (pid_t pid)
> +{
> + return netbsd_nat::pid_to_exec_file (pid);
> +}
> +
> +/* Implementation of the target_ops method "supports_pid_to_exec_file". */
> +
> +bool
> +netbsd_process_target::supports_pid_to_exec_file ()
> +{
> + return true;
> +}
> +
> +/* Implementation of the target_ops method "supports_hardware_single_step". */
> +bool
> +netbsd_process_target::supports_hardware_single_step ()
> +{
> + return true;
> +}
> +
> +/* Implementation of the target_ops method "sw_breakpoint_from_kind". */
> +
> +const gdb_byte *
> +netbsd_process_target::sw_breakpoint_from_kind (int kind, int *size)
> +{
> + static gdb_byte brkpt[PTRACE_BREAKPOINT_SIZE];
> +
> + *size = PTRACE_BREAKPOINT_SIZE;
> +
> + memcpy (brkpt, PTRACE_BREAKPOINT, PTRACE_BREAKPOINT_SIZE);

Is there a need to memcpy each time the function is called?

> +
> + return brkpt;
> +}
> +
> +/* Implement the thread_name target_ops method. */
> +
> +const char *
> +netbsd_process_target::thread_name (ptid_t ptid)
> +{
> + return netbsd_nat::thread_name (ptid);
> +}
> +
> +/* Implement the supports_catch_syscall target_ops method. */
> +
> +bool
> +netbsd_process_target::supports_catch_syscall ()
> +{
> + return true;
> +}
> +
> +/* Implement the supports_read_auxv target_ops method. */
> +
> +bool
> +netbsd_process_target::supports_read_auxv ()
> +{
> + return true;
> +}
> +
> +/* The NetBSD target ops object. */
> +
> +static netbsd_process_target the_netbsd_target;
> +
> +void
> +initialize_low ()
> +{
> + set_target_ops (&the_netbsd_target);
> + the_low_target.arch_setup ();
> +}
> diff --git a/gdbserver/netbsd-low.h b/gdbserver/netbsd-low.h
> new file mode 100644
> index 00000000000..2bd57c51726
> --- /dev/null
> +++ b/gdbserver/netbsd-low.h
> @@ -0,0 +1,157 @@
> +/* Copyright (C) 2020 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 GDBSERVER_NETBSD_LOW_H
> +#define GDBSERVER_NETBSD_LOW_H
> +
> +struct regcache;
> +struct target_desc;
> +
> +/* Some information relative to a given register set. */
> +
> +struct netbsd_regset_info
> +{
> + /* The ptrace request needed to get/set registers of this set. */
> + int get_request, set_request;
> + /* The size of the register set. */
> + int size;
> + /* Fill the buffer BUF from the contents of the given REGCACHE. */
> + void (*fill_function) (struct regcache *regcache, char *buf);
> + /* Store the register value in BUF in the given REGCACHE. */
> + void (*store_function) (struct regcache *regcache, const char *buf);

Leaving one blank line between each field may improve readability.

> +};
> +
> +/* A list of regsets for the target being debugged, terminated by an entry
> + where the size is negative.
> +
> + This list should be created by the target-specific code. */
> +
> +extern struct netbsd_regset_info netbsd_target_regsets[];
> +
> +/* The target-specific operations for NetBSD support. */
> +
> +struct netbsd_target_ops
> +{
> + /* Architecture-specific setup. */
> + void (*arch_setup) ();
> +
> + /* Hook to support target specific qSupported. */
> + void (*process_qsupported) (char **, int count);

Is the `process_qsupported` function called anywhere?

> +};

No strong opinion, but the low target could be a derived class of
netbsd_process_target, like the linux low targets.

> +
> +/* Target ops definitions for a NetBSD target. */
> +
> +class netbsd_process_target : public process_stratum_target
> +{
> +public:
> +
> + int create_inferior (const char *program,
> + const std::vector<char *> &program_args) override;
> +
> + void post_create_inferior () override;
> +
> + int attach (unsigned long pid) override;
> +
> + int kill (process_info *proc) override;
> +
> + int detach (process_info *proc) override;
> +
> + void mourn (process_info *proc) override;
> +
> + void join (int pid) override;
> +
> + bool thread_alive (ptid_t pid) override;
> +
> + void resume (thread_resume *resume_info, size_t n) override;
> +
> + ptid_t wait (ptid_t ptid, target_waitstatus *status,
> + int options) override;
> +
> + void fetch_registers (regcache *regcache, int regno) override;
> +
> + void store_registers (regcache *regcache, int regno) override;
> +
> + int read_memory (CORE_ADDR memaddr, unsigned char *myaddr,
> + int len) override;
> +
> + int write_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
> + int len) override;
> +
> + void request_interrupt () override;
> +
> + bool supports_read_auxv () override;
> +
> + int read_auxv (CORE_ADDR offset, unsigned char *myaddr,
> + unsigned int len) override;
> +
> + bool supports_hardware_single_step () override;
> +
> + const gdb_byte *sw_breakpoint_from_kind (int kind, int *size) override;
> +
> + bool supports_z_point_type (char z_type) override;
> +
> + int insert_point (enum raw_bkpt_type type, CORE_ADDR addr,
> + int size, struct raw_breakpoint *bp) override;
> +
> + int remove_point (enum raw_bkpt_type type, CORE_ADDR addr,
> + int size, struct raw_breakpoint *bp) override;
> +
> + bool stopped_by_sw_breakpoint () override;
> +
> + bool supports_qxfer_siginfo () override;
> +
> + int qxfer_siginfo (const char *annex, unsigned char *readbuf,
> + unsigned const char *writebuf, CORE_ADDR offset,
> + int len) override;
> +
> + bool supports_stopped_by_sw_breakpoint () override;
> +
> + bool supports_non_stop () override;
> +
> + bool supports_multi_process () override;
> +
> + bool supports_fork_events () override;
> +
> + bool supports_vfork_events () override;
> +
> + bool supports_exec_events () override;
> +
> + bool supports_disable_randomization () override;
> +
> + bool supports_qxfer_libraries_svr4 () override;
> +
> + int qxfer_libraries_svr4 (const char*, unsigned char*, const unsigned char*,
> + CORE_ADDR, int) override;
> +
> + bool supports_pid_to_exec_file () override;
> +
> + char *pid_to_exec_file (int pid) override;
> +
> + const char *thread_name (ptid_t thread) override;
> +
> + bool supports_catch_syscall () override;
> +};
> +
> +/* The inferior's target description. This is a global because the
> + NetBSD ports support neither bi-arch nor multi-process. */

The "nor multi-process" part of this comment is confusing. What does the multi-process
feature have to do with the_low_target? And netbsd_process_target was defined to
support multi-process above in the .cc file.

> +
> +extern struct netbsd_target_ops the_low_target;
> +
> +/* XXX: multilib */
> +extern const struct target_desc *netbsd_tdesc;
> +
> +#endif /* GDBSERVER_NETBSD_LOW_H */
> diff --git a/gdbserver/netbsd-x86_64-low.cc b/gdbserver/netbsd-x86_64-low.cc
> new file mode 100644
> index 00000000000..c8679d9f153
> --- /dev/null
> +++ b/gdbserver/netbsd-x86_64-low.cc
> @@ -0,0 +1,250 @@
> +/* Copyright (C) 2020 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/>. */
> +
> +#include <sys/types.h>
> +#include <sys/ptrace.h>
> +#include <limits.h>
> +
> +#include "server.h"
> +#include "netbsd-low.h"
> +#include "gdbsupport/x86-xstate.h"
> +#include "arch/amd64.h"
> +#include "x86-tdesc.h"
> +#include "tdesc.h"
> +
> +static int use_xml;
> +
> +/* The index of various registers inside the regcache. */
> +
> +enum netbsd_x86_64_gdb_regnum
> +{
> + AMD64_RAX_REGNUM, /* %rax */
> + AMD64_RBX_REGNUM, /* %rbx */
> + AMD64_RCX_REGNUM, /* %rcx */
> + AMD64_RDX_REGNUM, /* %rdx */
> + AMD64_RSI_REGNUM, /* %rsi */
> + AMD64_RDI_REGNUM, /* %rdi */
> + AMD64_RBP_REGNUM, /* %rbp */
> + AMD64_RSP_REGNUM, /* %rsp */
> + AMD64_R8_REGNUM, /* %r8 */
> + AMD64_R9_REGNUM, /* %r9 */
> + AMD64_R10_REGNUM, /* %r10 */
> + AMD64_R11_REGNUM, /* %r11 */
> + AMD64_R12_REGNUM, /* %r12 */
> + AMD64_R13_REGNUM, /* %r13 */
> + AMD64_R14_REGNUM, /* %r14 */
> + AMD64_R15_REGNUM, /* %r15 */
> + AMD64_RIP_REGNUM, /* %rip */
> + AMD64_EFLAGS_REGNUM, /* %eflags */
> + AMD64_CS_REGNUM, /* %cs */
> + AMD64_SS_REGNUM, /* %ss */
> + AMD64_DS_REGNUM, /* %ds */
> + AMD64_ES_REGNUM, /* %es */
> + AMD64_FS_REGNUM, /* %fs */
> + AMD64_GS_REGNUM, /* %gs */
> + AMD64_ST0_REGNUM = 24, /* %st0 */
> + AMD64_ST1_REGNUM, /* %st1 */
> + AMD64_FCTRL_REGNUM = AMD64_ST0_REGNUM + 8,
> + AMD64_FSTAT_REGNUM = AMD64_ST0_REGNUM + 9,
> + AMD64_FTAG_REGNUM = AMD64_ST0_REGNUM + 10,
> + AMD64_XMM0_REGNUM = 40, /* %xmm0 */
> + AMD64_XMM1_REGNUM, /* %xmm1 */
> + AMD64_MXCSR_REGNUM = AMD64_XMM0_REGNUM + 16,
> + AMD64_YMM0H_REGNUM, /* %ymm0h */
> + AMD64_YMM15H_REGNUM = AMD64_YMM0H_REGNUM + 15,
> + AMD64_BND0R_REGNUM = AMD64_YMM15H_REGNUM + 1,
> + AMD64_BND3R_REGNUM = AMD64_BND0R_REGNUM + 3,
> + AMD64_BNDCFGU_REGNUM,
> + AMD64_BNDSTATUS_REGNUM,
> + AMD64_XMM16_REGNUM,
> + AMD64_XMM31_REGNUM = AMD64_XMM16_REGNUM + 15,
> + AMD64_YMM16H_REGNUM,
> + AMD64_YMM31H_REGNUM = AMD64_YMM16H_REGNUM + 15,
> + AMD64_K0_REGNUM,
> + AMD64_K7_REGNUM = AMD64_K0_REGNUM + 7,
> + AMD64_ZMM0H_REGNUM,
> + AMD64_ZMM31H_REGNUM = AMD64_ZMM0H_REGNUM + 31,
> + AMD64_PKRU_REGNUM,
> + AMD64_FSBASE_REGNUM,
> + AMD64_GSBASE_REGNUM
> +};
> +
> +/* The fill_function for the general-purpose register set. */
> +
> +static void
> +netbsd_x86_64_fill_gregset (struct regcache *regcache, char *buf)
> +{
> + struct reg *r = (struct reg *)buf;

Space after ')'.

> +
> +#define netbsd_x86_64_collect_gp(regnum, fld) do { \
> + collect_register (regcache, regnum, &r->regs[_REG_##fld]); \
> + } while (0)
> +
> + netbsd_x86_64_collect_gp (AMD64_RAX_REGNUM, RAX);
> + netbsd_x86_64_collect_gp (AMD64_RBX_REGNUM, RBX);
> + netbsd_x86_64_collect_gp (AMD64_RCX_REGNUM, RCX);
> + netbsd_x86_64_collect_gp (AMD64_RDX_REGNUM, RDX);
> + netbsd_x86_64_collect_gp (AMD64_RSI_REGNUM, RSI);
> + netbsd_x86_64_collect_gp (AMD64_RDI_REGNUM, RDI);
> + netbsd_x86_64_collect_gp (AMD64_RBP_REGNUM, RBP);
> + netbsd_x86_64_collect_gp (AMD64_RSP_REGNUM, RSP);
> + netbsd_x86_64_collect_gp (AMD64_R8_REGNUM, R8);
> + netbsd_x86_64_collect_gp (AMD64_R9_REGNUM, R9);
> + netbsd_x86_64_collect_gp (AMD64_R10_REGNUM, R10);
> + netbsd_x86_64_collect_gp (AMD64_R11_REGNUM, R11);
> + netbsd_x86_64_collect_gp (AMD64_R12_REGNUM, R12);
> + netbsd_x86_64_collect_gp (AMD64_R13_REGNUM, R13);
> + netbsd_x86_64_collect_gp (AMD64_R14_REGNUM, R14);
> + netbsd_x86_64_collect_gp (AMD64_R15_REGNUM, R15);
> + netbsd_x86_64_collect_gp (AMD64_RIP_REGNUM, RIP);
> + netbsd_x86_64_collect_gp (AMD64_EFLAGS_REGNUM, RFLAGS);
> + netbsd_x86_64_collect_gp (AMD64_CS_REGNUM, CS);
> + netbsd_x86_64_collect_gp (AMD64_SS_REGNUM, SS);
> + netbsd_x86_64_collect_gp (AMD64_DS_REGNUM, DS);
> + netbsd_x86_64_collect_gp (AMD64_ES_REGNUM, ES);
> + netbsd_x86_64_collect_gp (AMD64_FS_REGNUM, FS);
> + netbsd_x86_64_collect_gp (AMD64_GS_REGNUM, GS);
> +}
> +
> +/* The store_function for the general-purpose register set. */
> +
> +static void
> +netbsd_x86_64_store_gregset (struct regcache *regcache, const char *buf)
> +{
> + struct reg *r = (struct reg *)buf;

Space after ')'.

> +
> +#define netbsd_x86_64_supply_gp(regnum, fld) do { \
> + supply_register (regcache, regnum, &r->regs[_REG_##fld]); \
> + } while(0)
> +
> + netbsd_x86_64_supply_gp (AMD64_RAX_REGNUM, RAX);
> + netbsd_x86_64_supply_gp (AMD64_RBX_REGNUM, RBX);
> + netbsd_x86_64_supply_gp (AMD64_RCX_REGNUM, RCX);
> + netbsd_x86_64_supply_gp (AMD64_RDX_REGNUM, RDX);
> + netbsd_x86_64_supply_gp (AMD64_RSI_REGNUM, RSI);
> + netbsd_x86_64_supply_gp (AMD64_RDI_REGNUM, RDI);
> + netbsd_x86_64_supply_gp (AMD64_RBP_REGNUM, RBP);
> + netbsd_x86_64_supply_gp (AMD64_RSP_REGNUM, RSP);
> + netbsd_x86_64_supply_gp (AMD64_R8_REGNUM, R8);
> + netbsd_x86_64_supply_gp (AMD64_R9_REGNUM, R9);
> + netbsd_x86_64_supply_gp (AMD64_R10_REGNUM, R10);
> + netbsd_x86_64_supply_gp (AMD64_R11_REGNUM, R11);
> + netbsd_x86_64_supply_gp (AMD64_R12_REGNUM, R12);
> + netbsd_x86_64_supply_gp (AMD64_R13_REGNUM, R13);
> + netbsd_x86_64_supply_gp (AMD64_R14_REGNUM, R14);
> + netbsd_x86_64_supply_gp (AMD64_R15_REGNUM, R15);
> + netbsd_x86_64_supply_gp (AMD64_RIP_REGNUM, RIP);
> + netbsd_x86_64_supply_gp (AMD64_EFLAGS_REGNUM, RFLAGS);
> + netbsd_x86_64_supply_gp (AMD64_CS_REGNUM, CS);
> + netbsd_x86_64_supply_gp (AMD64_SS_REGNUM, SS);
> + netbsd_x86_64_supply_gp (AMD64_DS_REGNUM, DS);
> + netbsd_x86_64_supply_gp (AMD64_ES_REGNUM, ES);
> + netbsd_x86_64_supply_gp (AMD64_FS_REGNUM, FS);
> + netbsd_x86_64_supply_gp (AMD64_GS_REGNUM, GS);
> +}
> +
> +/* Implements the netbsd_target_ops.arch_setup routine. */
> +
> +static void
> +netbsd_x86_64_arch_setup (void)
> +{
> + struct target_desc *tdesc
> + = amd64_create_target_description (X86_XSTATE_SSE_MASK, false, false, false);
> +
> + init_target_desc (tdesc, amd64_expedite_regs);
> +
> + netbsd_tdesc = tdesc;
> +}
> +
> +/* Update all the target description of all processes; a new GDB
> + connected, and it may or not support xml target descriptions. */
> +
> +static void
> +x86_64_netbsd_update_xmltarget (void)
> +{
> + struct thread_info *saved_thread = current_thread;
> +
> + /* Before changing the register cache's internal layout, flush the
> + contents of the current valid caches back to the threads, and
> + release the current regcache objects. */
> + regcache_release ();
> +
> + for_each_process ([] (process_info *proc) {
> + int pid = proc->pid;
> +
> + /* Look up any thread of this process. */
> + current_thread = find_any_thread_of_pid (pid);
> +
> + the_low_target.arch_setup ();

I find this confusing because the target object is supposed to be a singleton.
Why is its arch_setup called for each process?

> + });
> +
> + current_thread = saved_thread;
> +}
> +
> +/* Process qSupported query, "xmlRegisters=". */
> +
> +static void
> +netbsd_x86_64_process_qsupported (char **features, int count)
> +{
> + /* Return if gdb doesn't support XML. If gdb sends "xmlRegisters="
> + with "i386" in qSupported query, it supports x86 XML target
> + descriptions. */
> + use_xml = 0;
> + for (int i = 0; i < count; i++)
> + {
> + const char *feature = features[i];
> +
> + if (startswith (feature, "xmlRegisters="))
> + {
> + char *copy = xstrdup (feature + 13);
> + char *last;
> + char *p;
> +
> + for (p = strtok_r (copy, ",", &last); p != NULL;
> + p = strtok_r (NULL, ",", &last))
> + {
> + if (strcmp (p, "i386") == 0)
> + {
> + use_xml = 1;
> + break;
> + }
> + }
> +
> + free (copy);
> + }
> + }
> + x86_64_netbsd_update_xmltarget ();
> +}
> +
> +/* Description of all the x86-netbsd register sets. */
> +
> +struct netbsd_regset_info netbsd_target_regsets[] =
> +{
> + /* General Purpose Registers. */
> + {PT_GETREGS, PT_SETREGS, sizeof (struct reg),
> + netbsd_x86_64_fill_gregset, netbsd_x86_64_store_gregset},
> + /* End of list marker. */
> + {0, 0, -1, NULL, NULL }
> +};
> +
> +/* The netbsd_target_ops vector for x86-netbsd. */
> +
> +struct netbsd_target_ops the_low_target =
> +{
> + netbsd_x86_64_arch_setup,
> + netbsd_x86_64_process_qsupported,
> +};
> --
> 2.28.0

Regards
-Baris

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928