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
- From: "Paul Koning" <Paul_Koning at Dell dot com>
- To: "Pedro Alves" <pedro at codesourcery dot com>, <gdb-patches at sourceware dot org>
- Date: Mon, 3 May 2010 09:50:58 -0400
- Subject: RE: [PING] [RFC] Thread debug support for NetBSD 5
- 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> <201004291638.50078.pedro@codesourcery.com>
> > +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?
Probably true, I'll fix that.
> 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.
No, this variable is for reporting signals to gdb, so on a detach it
can/should be cleared.
> > + 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.
I copied that code out of inf-ptrace.c which is the common target code
for targets that use ptrace. NetBSD was using that before.
> > + /* 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.)
Right, I missed that when I added the debug messages. Thanks.
> > + /* 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.
Oops. I copied that from dec-thread.c.
paul