From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17804 invoked by alias); 19 Jun 2009 16:42:18 -0000 Received: (qmail 17785 invoked by uid 22791); 19 Jun 2009 16:42:15 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,J_CHICKENPOX_63,J_CHICKENPOX_93,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 19 Jun 2009 16:42:07 +0000 Received: (qmail 17200 invoked from network); 19 Jun 2009 16:42:05 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 19 Jun 2009 16:42:05 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [patch] gdbserver: Add qnx target Date: Fri, 19 Jun 2009 16:42:00 -0000 User-Agent: KMail/1.9.10 Cc: Aleksandar Ristovski References: In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200906191742.53849.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: 2009-06/txt/msg00513.txt.bz2 On Wednesday 17 June 2009 16:04:47, Aleksandar Ristovski wrote: > The patch is quite straight-forward. Not quite. > 2) gdbserver-ntotarget - ChangeLog > > * configure.srv (i[34567]86-*-nto*): New target. > * nto-low.c, nto-low.h, nto-x86-low.c: New files. > * remote-utils.c (include sys/iomgr.h): New include for > __QNX__ only. > (nto_comctrl): New function for __QNX__ only. > (enable_async_io, disable_async_io): Call nto_comcntrl for > __QNX__ only. I took a first look at this one. -- Pedro Alves On Wednesday 17 June 2009 16:04:47, Aleksandar Ristovski wrote: > +void nto_trace (const char *fmt, ...) Function name at column 0. > +void nto_trace (const char *fmt, ...) > +{ > + va_list arg_list; > + > + if (debug_threads == 0) > + return; > + ^ spurious space. > + printf ("nto:"); > + va_start (arg_list, fmt); > + vprintf (fmt, arg_list); > + va_end (arg_list); > +} This should go to stderr, not stdout. > +static void > +do_detach () ^ (void) > + return ptid_build (0, 0, 0); This is null_ptid, although minus_one_ptid is more common for returning errors. > +/* Given pid, open procfs path. Return ptid built from pid,0,tid. */ > + > +static ptid_t > +do_attach (ptid_t ptid) > +{ It would simpler and clearer if this took a simple `int pid' as argument. > + > +static void > +nto_resume (struct thread_resume *resume_info, size_t n) > +{ OOC, there's really no way to resume only one thread in nto then? (scheduler-locking). > + regcache_invalidate (); > +} You should flush the register cache *before* resuming, not after... > +static void nto_fetch_registers (int regno); Can you reorder the code so that forward declarations aren't needed? > +static ptid_t > +nto_wait (ptid_t ptid, > + struct target_waitstatus *ourstatus, int target_options) > +{ > + sigset_t set; > + siginfo_t info; > + procfs_status status; > + static int exit_signo = 0; /* For tracking exit status. */ Why is this static? > + > + TRACE ("%s\n", __func__); > + > + ourstatus->kind = TARGET_WAITKIND_SPURIOUS; > + > + if (ptid_equal (ptid, null_ptid)) > + { > + ourstatus->kind = TARGET_WAITKIND_STOPPED; > + ourstatus->value.sig = TARGET_SIGNAL_0; > + exit_signo = 0; > + TRACE ("null_ptid...\n"); > + return null_ptid; > + } Is this checking on null_ptid really needed? I'd prefer to not carry dead code from gdb into gdbserver... > + > + regcache_invalidate (); > + It is bogus to flush the regcache here to the inferior here, instead of just before resuming. > + > + return ptid_build (status.pid, status.tid, 0); > +} > + AFAICS, nowhere have you set the current_inferior. > + > +static void > +nto_fetch_registers (int regno) > +{ > + const unsigned int registeroffset > + = the_low_target.register_offset (regno); ^^^ bad indentation. > + const unsigned int registeroffset > + = the_low_target.register_offset (regno); Likewise. Spurious space after registeroffset. > +static void > +nto_store_registers (int regno) > +{ > + int reg; > + procfs_greg greg; > + int err; > + > + memset (&greg, 0, sizeof (greg)); > + > + TRACE ("%s (regno:%d)\n", __func__, regno); > + for (reg = 0; reg != the_low_target.num_regs; ++reg) > + { > + const unsigned int regoffset > + = the_low_target.register_offset (regno); > + collect_register (reg, ((char *)&greg) + regoffset); > + } > + if (greg.x86.eip != 0) > + err = devctl (nto_inferior.ctl_fd, DCMD_PROC_SETGREG, &greg, sizeof (greg), > + 0); > + reg = greg.x86.eip; > + TRACE ("eip=0x%08x\n", reg); > + if (err != EOK) > + TRACE ("Error: setting registers.\n"); > +} Okay, I'm confused. You're referencing x86 things here, in the nto-low.c file. > +static int > +nto_stopped_by_watchpoint (void) > +{ > + procfs_status status; > + int ret = 0; > + > + TRACE ("%s...", __func__); > + if (nto_inferior.ctl_fd != -1) > + { > + devctl (nto_inferior.ctl_fd, DCMD_PROC_STATUS, &status, sizeof (status), > + 0); > + if (status.flags & _DEBUG_FLAG_ISTOP) > + ret = 1; > + } > + ret = 0; ^^^^^^^ this seems bogus... > + TRACE ("%s\n", ret ? "yes" : "no"); > + return ret; > +} > + > +#ifdef __QNX__ > +static void > +nto_comctrl (int enable) > +{ > + struct sigevent event; > + > + if (enable) > + { > + event.sigev_notify = SIGEV_SIGNAL_THREAD; > + event.sigev_signo = SIGIO; > + event.sigev_code = 0; > + event.sigev_value.sival_ptr = NULL; > + event.sigev_priority = -1; > + ionotify (remote_desc, _NOTIFY_ACTION_POLLARM, _NOTIFY_COND_INPUT, > + &event); > + } > + else > + ionotify (remote_desc, _NOTIFY_ACTION_POLL, _NOTIFY_COND_INPUT, NULL); > +} > +#endif /* __QNX__ */ > + > + > @@ -828,6 +854,9 @@ enable_async_io (void) > signal (SIGIO, input_interrupt); > #endif > async_io_enabled = 1; > +#ifdef __QNX__ > + nto_comctrl (1); > +#endif /* __QNX__ */ > } > > /* Disable asynchronous I/O. */ > @@ -841,6 +870,10 @@ disable_async_io (void) > signal (SIGIO, SIG_IGN); > #endif > async_io_enabled = 0; > +#ifdef __QNX__ > + nto_comctrl (0); > +#endif /* __QNX__ */ > + > } > I don't really know a thing about nto/qnx apis, but, do you really need this nto_comctrl enable/disable calls? Can't you just do what nto_comctrl does once unconditionaly, and then rely on signal (SIGIO, SIG_IGN|input_interrupt), as you seem to already ? -- Pedro Alves