Re: [RFC/TileGX 3/6]fix syscall restart for hand call

[Jiong Wang <jiwang at tilera dot com>]

Hi Joel,

> > diff --git a/gdb/tilegx-linux-tdep.c b/gdb/tilegx-linux-tdep.c
> > index 64ddf00..7a44917 100644
> > --- a/gdb/tilegx-linux-tdep.c
> > +++ b/gdb/tilegx-linux-tdep.c
> > @@ -85,9 +85,11 @@ tilegx_linux_supply_regset (const struct regset *regset,
> >     int i;
> >   
> >     /* This logic must match that of struct pt_regs in "ptrace.h".  */
> > -  for (i = 0; i < TILEGX_NUM_EASY_REGS + 1; i++, ptr += tilegx_reg_size)
> > +  for (i = 0; i < TILEGX_NUM_EASY_REGS + 2; i++, ptr += tilegx_reg_size)
> >       {
> > -      int gri = (i < TILEGX_NUM_EASY_REGS) ? i : TILEGX_PC_REGNUM;
> > +      int gri = (i < TILEGX_NUM_EASY_REGS)
> > +                 ? i : (i == TILEGX_NUM_EASY_REGS)
> > +                        ? TILEGX_PC_REGNUM : TILEGX_FAULTNUM_REGNUM;
> I am not a huge fan of the approach taken (the "+2" and associated
> double-layer of ?: ternary operators). But OK if you really want to
> keep it that way. I kind of see why it is the way it is. One possible
> suggestion, at your option, is to use a case statement for gri.
> I personally think it's clearer, and going to scale better if you
> ever had to add more registers.

    thanks for suggestion, but I prefer to keep it this way currently.
> > +#define INT_SWINT_1_SIGRETURN (~0)
> Can you provide a comment explaining what this macro provides?
>
> > +static void
> > +tilegx_write_pc (struct regcache *regcache, CORE_ADDR pc)
> All new functions should be documented. In this case, since this
> is a gdbarch "method", you can just say:
>
> /* Implement the "write_pc" gdbarch method.  */
>
> Also, you may know this already, but just in case, can you leave
> an empty line between that comment and the start of the function
> definition?
>
> > +  regcache_cooked_write_unsigned (regcache, TILEGX_PC_REGNUM, pc);
> > +
> > +  /* We must be careful with modifying the program counter.  If we
> > +     just interrupted a system call, the kernel might try to restart
> > +     it when we resume the inferior.  On restarting the system call,
> > +     the kernel will try backing up the program counter even though it
> > +     no longer points at the system call.  This typically results in a
> > +     SIGSEGV or SIGILL.  We can prevent this by writing INT_SWINT_1_SIGRETURN
> > +     in the "faultnum" pseudo-register.
> > +
> > +     Note that "faultnum" is saved when setting up a dummy call frame.
> > +     This means that it is properly restored when that frame is
> > +     popped, and that the interrupted system call will be restarted
> > +     when we resume the inferior on return from a function call from
> > +     within GDB.  In all other cases the system call will not be
> > +     restarted.  */
> > +  regcache_cooked_write_unsigned (regcache, TILEGX_FAULTNUM_REGNUM,
> > +                                  INT_SWINT_1_SIGRETURN);
> > +}
> > +
> > +
> > +
> >   /* This is the implementation of gdbarch method breakpoint_from_pc.  */
> Too many empty lines before this comment. Can you remove 2 and leave
> only 1?

    all fixed, is the updated patch OK?

     and I found this bug also existed on ARM tagret
       http://sourceware.org/bugzilla/show_bug.cgi?id=10267
>

Attachment: 03-revise.patch
Description: Text document