[PATCH] Add basic event handling in the NetBSD target

[Kamil Rytarowski]

On 24.04.2020 03:33, Simon Marchi wrote:
> Hi Kamil,
> 
> Check the indentation throughout the patch, some lines have 1 space indent instead of 2.
> 

OK.

> 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.
> 

OK.

>> 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?
> 

In Linux there can be non-stop mode and threads are managed (stopped,
started, etc) individually. On NetBSD we can start and stop the whole
process, regardless of the number of threads in it.

We need to set the state of all threads before PT_CONTINUE (or assume
unchanged state of them).

If a thread is not expected to be suspended, we shall set its flag to
resume. The same for single-step (PT_STEP). This approach simplifies the
implementation as I don't need to track internally which thread has what
status and merely save a few syscalls.

>> +    }
>> +   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.
> 

The caller can pass minus_one_ptid, which makes no sense for NetBSD and
certainly in the multi-target support.

Scenarios, as I can see them on NetBSD:

 - passing ptid_t (pid, 0, 0) -> resume the whole process with all the
threads
 - passing ptid_t (pid, lwp, 0) -> resume pid::lwp with all the other
threads suspended in pid (`set scheduler-lock')
 - passing ptid_t (-1, ?, ?) -> confusion to me so I try to find any
fallback

If we can abandon the -1 case, I can drop the inferior_ptid usage. If
the core no longer passes -1 (it could be), we shall cleanup existing
code that handles this as it could happen. If a refactoring can happen,
this shall be followed by addition of gdb_assert(ptid != minus_one_ptid).

>> +    }
>> +
>> +  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.
> 

I will fix.

> 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?
> 

The core code asks to waitpid() with WNOHANG, receives pid=0 (nobody)
and crashes.

It's a generic bug that shall be fixed... but it looks like nobody
really obeys the core and the 3rd argument is overwritten (compare:
rs6000-nat.c, obsd-nat.c, inf-ptrace.c or Linux...).

I've landed into more similar generic nuances during my effort on GDB,
that handling events promptly can crash the core and I need to change
the behavior to emulate other kernels.

It would be better to fix the core, but it is not NetBSD's fault and not
a blocker for NetBSD support.

If there is interest in fixing it, I can file a bug report
independently. I don't pledge to work on it myself (at least as long as
I can merely focus on one kernel support).

>> +    }
>> +  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.
> 

OK.

>> +
>> +  /* 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.
> 

OK.

>> 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?
> 

I will fix it.

> Simon
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://sourceware.org/pipermail/gdb-patches/attachments/20200424/73a6096e/attachment.sig>