This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] gdbserver: Add qnx target


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]