Pedro Alves wrote:
> 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.
> 


 > +void nto_trace (const char *fmt, ...)

Pedro: Function name at column 0.

Done.

 > > +void nto_trace (const char *fmt, ...)
 > > +{
 > > +  va_list arg_list;
 > > +
 > > +  if (debug_threads == 0)
 > > +    return;
 > > +

Pedro:   ^ spurious space.

Done.

 > > +  printf ("nto:");
 > > +  va_start (arg_list, fmt);
 > > +  vprintf (fmt, arg_list);
 > > +  va_end (arg_list);
 > > +}

Pedro: This should go to stderr, not stdout.

Done.

 > > +static void
 > > +do_detach ()
               ^ (void)


 > > +      return ptid_build (0, 0, 0);

Pedro: This is null_ptid, although minus_one_ptid is
more common for returning errors.

Done

 > > +/* Given pid, open procfs path.  Return ptid built 
from pid,0,tid.  */
 > > +
 > > +static ptid_t
 > > +do_attach (ptid_t ptid)
 > > +{

Pedro: It would simpler and clearer if this took a
simple `int pid' as argument.

Done.

 > > +
 > > +static void
 > > +nto_resume (struct thread_resume *resume_info, size_t n)
 > > +{

Pedro: OOC, there's really no way to resume only one thread in
nto then? (scheduler-locking).

Not for now, but it will soon.

 > > +  regcache_invalidate ();
 > > +}

Pedro: You should flush the register cache *before* 
resuming, not
after...


Done.

 > > +static void nto_fetch_registers (int regno);

Pedro: Can you reorder the code so that forward declarations
aren't needed?

This forward decl was not necessary. Removed.

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

It should really go into "nto_inferior" structure. I plan to 
support multiple processes so there will be a chance for 
cleanups.

 > > +
 > > +  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;
 > > +    }

Pedro: Is this checking on null_ptid really needed?  I'd prefer
to not carry dead code from gdb into gdbserver...

Removed.

 > > +
 > > +  regcache_invalidate ();
 > > +

Pedro: It is bogus to flush the regcache here to the inferior
here, instead of just before resuming.

Ok, moved before resume.

 > > +
 > > +  return ptid_build (status.pid, status.tid, 0);
 > > +}
 > > +

Pedro: AFAICS, nowhere have you set the current_inferior.

add_thread will set it (see inferiors.c:184)


 > > +
 > > +static void
 > > +nto_fetch_registers (int regno)
 > > +{

 > > +             const unsigned int registeroffset
 > > +                     = the_low_target.register_offset 
(regno);

Pedro:                   ^^^ bad indentation.

Changed.


 > > +         const unsigned int registeroffset
 > > +                 = the_low_target.register_offset (regno);

Pedro: Likewise.  Spurious space after registeroffset.

Removed.


 > > +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");
 > > +}

Pedro: Okay, I'm confused.  You're referencing x86 things 
here, in the
nto-low.c file.

No wonder you are confused :-)  It was a leftover from my 
debugging. Removed.

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

Pedro:     ^^^^^^^ this seems bogus...

Another leftover.. Removed.

 > > +  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__ */
 > > +
 > >  }
 > >

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.


I added support for watchpoints.

Thanks,

-- 
Aleksandar Ristovski
QNX Software Systems

ChangeLog remains the same:


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