Re: [PING] [RFC] Thread debug support for NetBSD 5

[Pedro Alves <pedro at codesourcery dot com>]

On Thursday 29 April 2010 15:49:16, Paul Koning wrote:
> Ping... any comments?  

(I know nothing about NetBSD threads support)

> 2010-04-22  Paul Koning  <paul_koning@dell.com>
> 
> 	* i386bsd-nat.c (i386bsd_supply_gregset, i386bsd_collect_gregset):
> 	Make global.
> 	* i386bsd-nat.h: Ditto.
> 	* i386nbsd-nat.c: Include inferior.h, i387-tdep.h, sys/ptrace.h,
> 	machine/reg.h.
> 	(i386nbsd_fetch_inferior_registers,
> 	i386nbsd_store_inferior_registers): New.
> 	* mipsnbsd-nat.c (mipsnbsd_fetch_inferior_registers,
> 	mipsnbsd_store_inferior_registers): Pass thread ID to ptrace().


> 	* nbsd-thread.c: New file.

I took a look at this file, only.  Mark would probably
be the most qualified to review the rest of the patch.


> 	* config/i386/nbsdelf.mh: Add nbsd-thread.o.
> 	* config/mips/nbsd.mh: Add nbsd-thread.o.

> Index: gdb/nbsd-thread.c
> ===================================================================
> RCS file: gdb/nbsd-thread.c
> diff -N gdb/nbsd-thread.c
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ gdb/nbsd-thread.c	22 Apr 2010 15:21:55 -0000
> @@ -0,0 +1,698 @@

> +/* Count of signals still pending delivery to GDB.  These are threads
> +   that were found to be stopped and not breakpoints.  For threads that
> +   hit a breakpoint, we simply push back the thread so it will hit the
> +   break again (if it isn't removed before then) but for other signals,
> +   for example faults, the signal remains pending, the "to_resume" that
> +   resumes the whole process is skipped, and then the "to_wait" returns
> +   the information about one of the pending signals instead.  */
> +static int pending_sigs;

I'm not sure whether this global can get stale between
debug sessions or not, but it looked like it.  Say, if you
kill a process while you have pending sigs, the next
debug session will trip on it being != 0?  It also points
out that you should probably do something to the pending
signals when you go about detaching from a process, so
they don't get lost.


> +/* Deactivate thread support.  Do nothing is thread support is
> +   already inactive.  */

Typo: s/is thread/if thread/

> +  if (catch_syscall_enabled () > 0)
> +    request = PT_SYSCALL;
> +  else
> +    request = PT_CONTINUE;

I think this will be dead code, since you don't
support inserting catch syscalls.

> +  /* An address of (PTRACE_TYPE_ARG3)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.  */
> +  errno = 0;

If this clearing of errno is needed, then it should move to just
before the `ptrace' calls.  You have several function calls between
this and the `ptrace' calls (at least when debugging is enable),
and any of those could clobber `errno'.  (That's the reason
for `save_errno' in your patch somewhere else, BTW.)


> +      /* If nothing found in the no wait case, report that.  */
> +      if (options == WNOHANG && pid == 0)
> +	return pid_to_ptid (-1);

Use minus_one_ptid, here and everywhere else.


> +static char *
> +nbsd_thread_pid_to_str (struct target_ops *ops, ptid_t ptid)
> +{
> +  if (TIDGET (ptid) == 0)
> +    {
> +      struct target_ops *beneath = find_target_beneath (ops);
> +
> +      return beneath->to_pid_to_str (beneath, ptid);
> +    }
> +  return xstrprintf (_("Thread %ld"), TIDGET (ptid));

This leaks.  Nothing ever releases the return of
target_pid_to_str calls; that's why all implementations
return a pointer to a static buffer.

> +static void
> +init_nbsd_thread_ops (void)
> +{
> +  nbsd_thread_ops.to_shortname          = "netbsd-threads";
> +  nbsd_thread_ops.to_longname           = _("NetBSD threads support");
> +  nbsd_thread_ops.to_doc                = _("NetBSD threads support");
> +  nbsd_thread_ops.to_detach             = nbsd_thread_detach;
> +  nbsd_thread_ops.to_resume             = nbsd_thread_resume;
> +  nbsd_thread_ops.to_wait               = nbsd_thread_wait;
> +  nbsd_thread_ops.to_mourn_inferior     = nbsd_thread_mourn_inferior;
> +  nbsd_thread_ops.to_thread_alive       = nbsd_thread_thread_alive;
> +  nbsd_thread_ops.to_pid_to_str         = nbsd_thread_pid_to_str;
> +  nbsd_thread_ops.to_stratum            = thread_stratum;
> +  nbsd_thread_ops.to_magic              = OPS_MAGIC;
> +}

I'd really prefer to get rid of the vertical alignment, and
put just one space before and after the `='.  I was bitten
before for greeping for e.g., "to_detach = " when adjusting
all targets for an interface change, and not finding
some uses like this one, and thus ending up breaking
the build for some targets.

> +
> +void
> +_initialize_nbsd_thread (void)

Please provide a prototype for this.  E.g.,:

 /* Provide a prototype to silence -Wmissing-prototypes.  */
 extern initialize_file_ftype _initialize_linux_nat;

Otherwise, looks fine to me.

-- 
Pedro Alves