From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26336 invoked by alias); 20 Jun 2009 00:01:44 -0000 Received: (qmail 25906 invoked by uid 22791); 20 Jun 2009 00:01:43 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,J_CHICKENPOX_35,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; Sat, 20 Jun 2009 00:01:38 +0000 Received: (qmail 6814 invoked from network); 20 Jun 2009 00:01:35 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 20 Jun 2009 00:01:35 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [patch] gdbserver: Add qnx target Date: Sat, 20 Jun 2009 00:01:00 -0000 User-Agent: KMail/1.9.10 Cc: Aleksandar Ristovski References: <200906191742.53849.pedro@codesourcery.com> <4A3BEDD6.6070904@qnx.com> In-Reply-To: <4A3BEDD6.6070904@qnx.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200906200102.25411.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/msg00522.txt.bz2 On Friday 19 June 2009 20:58:14, Aleksandar Ristovski wrote: > Pedro Alves wrote: > > > +static void > > > +do_detach () > ^ (void) > You missed this one. > > > +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. */ > > Pedro: Why is this static? > > We first register that signal has hit the inferior, but it > hasn't died just yet. We set exit_signo and return > TARGET_WAITKIND_STOPPED. On next resume it really > terminates, we set recorded signal from the static. Ah, I see. > > It should really go into "nto_inferior" structure. I plan to > support multiple processes so there will be a chance for > cleanups. I don't see that it would take more than a few lines of code to move this to struct nto_inferior. It looks to me that you could set the exit signo here instead: nto_resume: if (status.why & (_DEBUG_WHY_SIGNALLED | _DEBUG_WHY_FAULTED)) { if (signal_to_pass != status.info.si_signo) { kill (status.pid, signal_to_pass); run.flags |= _DEBUG_RUN_CLRFLT | _DEBUG_RUN_CLRSIG; } >>> else /* Let it kill the program without telling us. */ >>> sigdelset (&run.trace, signal_to_pass); < } (and always clear it on entry to nto_resume) ... but I may be wrong. It's OK to leave as is (I don't care *that* much about this file ;-) ), but if you want to leave as is, then please paste that explanation in the code. > Pedro: AFAICS, nowhere have you set the current_inferior. > > add_thread will set it (see inferiors.c:184) How does that help in the nto_wait case I pointed? You have to set current_inferior to the thread that is reporting the event, otherwise, the following register reads may read from the wrong thread (*). Question: did you run the testsuite against this? I'd be curious to know how does this compares against native gdb. (*) - unless I'm missing/forgetting something. Sounds like something we can now assure / clean up in generic code, since target_wait now returns a ptid_t... > Pedro: 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 ? > > > Unfortunately, I couldn't find a way. Whatever I looked at > requires that handle is present. The problem here is that > while we are blocked, we will not receive SIGIO signal > unless we explicitly setup an event. > And we need to do it > every time before we go into sigwaitinfo. Ok, I peeked at ionotify's docs, and from what I've understood, this is fine. I was considering if this wasn't racy, but it seems like ionotify (_NOTIFY_ACTION_POLLARM) behaves a bit like select --- if there's data already there, it triggers an action immediately. I'm not all that happy with having __QNX__ wrapped code, but it's mostly contained, and not really worse than USE_WIN32API... If there's more of these to come, than let's think about abstracting out the posix|win32|qnx initialize|disable|enable_async_io functions to separate files somehow. Comments on the new patch: > +static int > +nto_stopped_by_watchpoint (void) > +{ > + procfs_status status; > + int ret = 0; > + const int watchmask = _DEBUG_FLAG_TRACE_RD | _DEBUG_FLAG_TRACE_WR > + | _DEBUG_FLAG_TRACE_MODIFY; > + > + TRACE ("%s...", __func__); > + if (nto_inferior.ctl_fd != -1) > + { > + int err = devctl (nto_inferior.ctl_fd, DCMD_PROC_STATUS, &status, > + sizeof (status), 0); ^^^^^^ tab vs space > +} > + > + You used 1 line spacing everywhere else. > +static int > +nto_supports_non_stop (void) > +{ > + TRACE ("%s\n", __func__); > + return 0; > +} > + > + > + This function isn't really needed though. The default is 0. (Not installing such callbacks may make life easier for someone else in the future, if e.g., the interface of such functions needs to change, but it's no biggie.) > + int (*register_offset)(int gdbregno); ^ missing space > +/* Activated by env. variable GDBSERVER_DEBUG. */ > +extern int nto_debug; This isn't defined anywhere. Please remove. > * 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. There's a standard way to mention these conditionalized changes. It goes something like this: * remote-utils.c [__QNX__]: Include sys/iomgr.h. (nto_comctrl) [__QNX__]: New function. (enable_async_io, disable_async_io) [__QNX__]: Call nto_comcntrl. Other than the above remarks, this is good to go, once its dependencies are in. -- Pedro Alves