This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PING] [RFC] Thread debug support for NetBSD 5
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