Re: [RFA 3/3] gdbserver support for powerpc-lynxos (4.x)

[Pedro Alves <pedro@codesourcery.com>]

On Tuesday 31 August 2010 20:37:31, Joel Brobecker wrote:
>     Note that I'm adding a new varialble "srv_extra_libs" in configure.srv.
>     I need this because I need to link against -linet. Perhaps the right way
>     of doing things might have been to use a configure check...

I think this comment is stale.

> +      status->kind = TARGET_WAITKIND_EXITED;
> +      status->value.integer = target_signal_from_host (WEXITSTATUS (wstat));

This target_signal_from_host call is wrong here.  The program's exit status
is not a signal (see gdb/target.c:store_waitstatus).

> +  /* SIGTRAP events are generated for situations other than single-step/
> +     breakpoint events (Eg. new-thread events).  Handle those other types
> +     of events, and resume the execution if necessary.  */
> +  if (status->kind == TARGET_WAITKIND_STOPPED
> +      && status->value.integer == target_signal_from_host (SIGTRAP))

Just write TARGET_SIGNAL_TRAP.

> +static int
> +lynx_kill (int pid)
> +{
> +  ptid_t ptid = lynx_ptid_build (pid, 0);
> +  struct target_waitstatus status;
> +
> +  lynx_ptrace (PTRACE_KILL, ptid, 0, 0, 0);
> +  lynx_wait (ptid, &status, 0);
> +  return 0;
> +}

Should call the_target->mourn (process);

> +/* Implement the detach target_ops method.  */
> +
> +static int
> +lynx_detach (int pid)
> +{
> +  ptid_t ptid = lynx_ptid_build (pid, 0);
> +
> +  lynx_ptrace (PTRACE_DETACH, ptid, 0, 0, 0);
> +  return 0;
> +}

Should call the_target->mourn (process);

> +      buf = lynx_ptrace (PTRACE_PEEKTEXT, inferior_ptid, addr, 0, 0);
> +      if (errno)
> +        return errno;

lynx_ptrace is not taking care of preserving the errno set by the
ptrace call:

> +  result = ptrace (request, pid, addr, data, addr2);
> +  if (debug_threads)
> +    printf (" -> %d (=0x%x)\n", result, result);
> +
> +  return result;
> +}

... that printf call clobbers errno when debug_threads is on.  And
you should use "fprintf (stderr, " for debug output throughout.

> +      memcpy (myaddr + (addr - memaddr) + skip, (gdb_byte *)&buf + skip,

You should have a space after the cast (there are more instances of this).

Other than that it looks fine to me.

-- 
Pedro Alves