Re: [patch] gdbserver: Add qnx target

[Pedro Alves <pedro at codesourcery dot com>]

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