Pedro Alves wrote:
> On Friday 19 June 2009 20:58:14, Aleksandar Ristovski wrote:
>> Pedro Alves wrote:
>>  > > +static void
>>  > > +do_detach ()
>>                ^ (void)
>>
> 
> You missed this one.

ok.

> 
>>  > > +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.  

Moved.

> 
> 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.

Didn't change this.


> 
>> 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...

I think your feeling was right: I did not have code for 
adding new threads. Rectified now - thanks for all your help!

> 
>> 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.

Ok, I *think* I addressed all spacing/tabbing issues.

> 
>> +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.)

I would leave it as a testimony to my commitment to enable 
non-stop on Neutrino. :-)

> 
>> +  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.
> 

Ok, here is my new patch. I addressed all of the above, and 
probably introduced some new issues :-).  For my bonus 
points, I added comments for each function definition in 
nto-low.c

Let me know what you think (once this goes in, I will change 
gdb's configure.tgt to say "yes" to generating gdbserver for 
Neutrino - in a separate patch submission).

Thanks,

-- 
Aleksandar Ristovski
QNX Software Systems


ChangeLog (NOTE: I merged both configuration/Makefile 
changes and new files into one all-inclusive patch).

* configure: Regenerated.
* configure.ac: Add case for srv_qnx and set LIBS 
accordingly..
* configure.srv (i[34567]86-*-nto*): New target.
* nto-low.c, nto-low.h, nto-x86-low.c: New files.
* remote-utils.c [__QNX__]: Include sys/iomgr.h
(nto_comctrl) [__QNX__]: New function.
(enable_async_io, disable_async_io) [__QNX__]: Call nto_comctrl.