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

[Joel Brobecker <brobecker at adacore dot com>]

On Fri, Jan 18, 2013 at 05:26:52PM +0800, Jiong Wang wrote:
> like x86, 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.
> 
> gdb/ChangeLog:
> 
>         * tilegx-tdep.c (tilegx_write_pc): New function.
>         prevent kernel from restarting syscall for gdb hand call.
>         (tilegx_cannot_reference_register): Likewise.
>         (tilegx_gdbarch_init): Likewise.
>         (tilegx_register_name): Likewise.
>         * tilegx-tdep.h (enum tilegx_regnum): Update.
>         * tilegx-linux-tdep.c (tilegx_linux_supply_regseit): Likewise.
>         * tilegx-linux-nat.c (regmap): Update.

Questions below...

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

> +#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?



-- 
Joel