This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC/TileGX 3/6]fix syscall restart for hand call
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Jiong Wang <jiwang at tilera dot com>
- Cc: gdb-patches at sourceware dot org, Walter Lee <walt at tilera dot com>
- Date: Fri, 18 Jan 2013 17:33:59 +0400
- Subject: Re: [RFC/TileGX 3/6]fix syscall restart for hand call
- References: <50F9155C.1090703@tilera.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