From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27209 invoked by alias); 29 Apr 2010 15:39:04 -0000 Received: (qmail 27172 invoked by uid 22791); 29 Apr 2010 15:38:59 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 29 Apr 2010 15:38:55 +0000 Received: (qmail 29783 invoked from network); 29 Apr 2010 15:38:53 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 29 Apr 2010 15:38:53 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [PING] [RFC] Thread debug support for NetBSD 5 Date: Thu, 29 Apr 2010 15:39:00 -0000 User-Agent: KMail/1.12.2 (Linux/2.6.31-20-generic; KDE/4.3.2; x86_64; ; ) Cc: Paul Koning References: <19405.52446.728141.329821@pkoning-laptop.equallogic.com> <19408.27827.830391.509465@pkoning-laptop.equallogic.com> <19417.40044.762978.858637@pkoning-laptop.equallogic.com> In-Reply-To: <19417.40044.762978.858637@pkoning-laptop.equallogic.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201004291638.50078.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-04/txt/msg00956.txt.bz2 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 > > * 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