[PATCH] Add basic event handling in the NetBSD target

[Simon Marchi]

Hi Kamil,

Check the indentation throughout the patch, some lines have 1 space indent instead of 2.

On 2020-04-17 10:45 a.m., Kamil Rytarowski wrote:
> Implement the following events:
>  - single step (TRAP_TRACE)
>  - software breakpoint (TRAP_DBREG)
>  - exec() (TRAP_EXEC)
>  - syscall entry/exit (TRAP_SCE / TRAP_SCX)
> 
> Add support for NetBSD specific ::wait () and ::resume ().
> 
> Instruct the generic code that exec and syscall events are supported.
> 
> Define an empty nbsd_get_syscall_number as it is prerequisite for
> catching syscall entry and exit events, even if it is unused.
> This function is used to detect whether the gdbarch supports the
> 'catch syscall' feature.
> 
> gdb/ChangeLog:
> 
>        * nbsd-nat.c: Include "sys/wait.h".
>        * (nbsd_nat_target::resume, nbsd_wait, nbsd_nat_target::wait)

Remove asterisk on last line.

> diff --git a/gdb/nbsd-nat.c b/gdb/nbsd-nat.c
> index d41cfc815d3..f9e85e10b16 100644
> --- a/gdb/nbsd-nat.c
> +++ b/gdb/nbsd-nat.c
> @@ -28,6 +28,7 @@
>  #include <sys/types.h>
>  #include <sys/ptrace.h>
>  #include <sys/sysctl.h>
> +#include <sys/wait.h>
> 
>  /* Return the name of a file that can be opened to get the symbols for
>     the child process identified by PID.  */
> @@ -539,3 +540,213 @@ nbsd_nat_target::info_proc (const char *args, enum info_proc_what what)
> 
>    return true;
>  }
> +
> +/* Resume execution of thread PTID, or all threads if PTID is -1.  If
> +   STEP is nonzero, single-step it.  If SIGNAL is nonzero, give it
> +   that signal.  */
> +
> +void
> +nbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signal)
> +{
> +  int request;
> +
> +  if (ptid.lwp_p ())
> +    {
> +      /* If ptid is a specific LWP, suspend all other LWPs in the process.  */
> +      inferior *inf = find_inferior_ptid (this, ptid);
> +
> +      for (thread_info *tp : inf->non_exited_threads ())
> +        {
> +          if (tp->ptid.lwp () == ptid.lwp ())
> +            request = PT_RESUME;
> +          else
> +            request = PT_SUSPEND;
> +
> +          if (ptrace (request, tp->ptid.pid (), NULL, tp->ptid.lwp ()) == -1)
> +            perror_with_name (("ptrace"));
> +        }

I know that you'll say it's because FreeBSD does it this way, but still it surprised
me.  If the core of GDB asks to resume one specific thread, it doesn't mean you should
suspend the other threads, they should just be left in whatever state they are.  Is there
a reason to stop the other threads here?

> +    }
> +   else
> +    {
> +      /* If ptid is a wildcard, resume all matching threads (they won't run
> +         until the process is continued however).  */
> +      for (thread_info *tp : all_non_exited_threads (this, ptid))
> +        if (ptrace (PT_RESUME, tp->ptid.pid (), NULL, tp->ptid.lwp ()) == -1)
> +          perror_with_name (("ptrace"));
> +      ptid = inferior_ptid;

Can you explain this?  Ideally the resume method should not rely on inferior_ptid.  We
are working on reducing the dependencies on this global, in favor of passing the context
by parameters.  Here, the `ptid` parameter should be enough.

> +    }
> +
> +  if (step)
> +    {
> +      for (thread_info *tp : all_non_exited_threads (this, ptid))
> +       if (ptrace (PT_SETSTEP, tp->ptid.pid (), NULL, tp->ptid.lwp ()) == -1)
> +         perror_with_name (("ptrace"));
> +     }
> +  else
> +    {
> +      for (thread_info *tp : all_non_exited_threads (this, ptid))
> +       if (ptrace (PT_CLEARSTEP, tp->ptid.pid (), NULL, tp->ptid.lwp ()) == -1)
> +         perror_with_name (("ptrace"));
> +    }
> +
> +  if (minus_one_ptid == ptid)
> +    /* Resume all threads.  Traditionally ptrace() only supports
> +       single-threaded processes, so simply resume the inferior.  */
> +    ptid = ptid_t (inferior_ptid.pid ());
> +
> +  if (catch_syscall_enabled () > 0)
> +    request = PT_SYSCALL;
> +  else
> +    request = PT_CONTINUE;
> +
> +  /* An address of (void *)1 tells ptrace to continue from
> +     where it was.  If GDB wanted it to start some other way, we have
> +     already written a new program counter value to the child.  */
> +  if (ptrace (request, ptid.pid (), (void *)1, gdb_signal_to_host (signal)) == -1)
> +    perror_with_name (("ptrace"));
> +}
> +
> +/* Implement the "update_thread_list" target_ops method.  */
> +
> +static ptid_t
> +nbsd_wait (ptid_t ptid, struct target_waitstatus *ourstatus, int options)

The comment doesn't match the function.

Since this function only returns a pid, I think it would be clearer if it returned
an int or pid_t.  At first glance, I didn't see that it returned a ptid_t with only
the pid field filled.  If the caller needs to transform it into a ptid, it can do that.

> +{
> +  pid_t pid;
> +  int status;
> +
> +  set_sigint_trap ();
> +
> +  do
> +    {
> +      /* The common code passes WNOHANG that leads to crashes, overwrite it.  */
> +      pid = waitpid (ptid.pid (), &status, 0);

What is crashing exactly?

> +    }
> +  while (pid == -1 && errno == EINTR);
> +
> +  clear_sigint_trap ();
> +
> +  if (pid == -1)
> +    perror_with_name (_("Child process unexpectedly missing"));
> +
> +  store_waitstatus (ourstatus, status);
> +  return ptid_t (pid);
> +}
> +
> +/* 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.  */
> +
> +ptid_t
> +nbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
> +		       int target_options)
> +{
> +  ptid_t wptid = nbsd_wait(ptid, ourstatus, target_options);

Missing space.

> +
> +  /* If the child stopped, keep investigating its status.  */
> +  if (ourstatus->kind != TARGET_WAITKIND_STOPPED)
> +    return wptid;
> +
> +  pid_t pid = wptid.pid ();
> +
> +  /* 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;
> +
> +  int 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..  */
> +  if (signo != SIGTRAP)
> +    return wptid;
> +
> +  /* Stop examining non-debugger oriented SIGTRAP codes.  */
> +  if (code <= SI_USER || code == SI_NOINFO)
> +    return wptid;
> +
> +  if (in_thread_list (this, ptid_t (pid)))
> +    {
> +      thread_change_ptid (this, ptid_t (pid), wptid);
> +    }

Curly braces not needed.

> diff --git a/gdb/nbsd-tdep.c b/gdb/nbsd-tdep.c
> index 52e0640e35c..d5d1b7211c1 100644
> --- a/gdb/nbsd-tdep.c
> +++ b/gdb/nbsd-tdep.c
> @@ -444,6 +444,21 @@ nbsd_info_proc_mappings_entry (int addr_bit, ULONGEST kve_start,
>      }
>  }
> 
> +/* Implement the "get_syscall_number" gdbarch method.  */
> +
> +static LONGEST
> +nbsd_get_syscall_number (struct gdbarch *gdbarch, thread_info *thread)
> +{
> +
> +  /* FreeBSD doesn't use gdbarch_get_syscall_number since NetBSD
> +     native targets fetch the system call number from the
> +     'si_sysnum' member of siginfo_t in nbsd_nat_target::wait.
> +     However, system call catching requires this function to be
> +     set.  */

FreeBSD?

Simon