Re: [PATCH 2/2] GDB process record and reverse debugging improvements for arm*-linux*

[Pedro Alves <palves@redhat.com>]

On 11/24/2013 11:47 PM, Omair Javaid wrote:

> After incorporating all suggestions I am posting a final patch. Looking
> for a go ahead for commit.
> 
> This patch adds support for process record/replay system call recording 
> for arm targets.
> 

> 2013-11-08  Omair Javaid  <omair.javaid@linaro.org>
> 
> 	* arm-linux-tdep.c (struct arm_linux_record_tdep): Declare.
> 	(arm_canonicalize_syscall): New function.
> 	(arm_all_but_pc_registers_record): New function.
> 	(arm_linux_syscall_record): New function.
> 	(arm_linux_init_abi): Add syscall recording constructs.
...

> 	* arm-linux-tdep.c: Include "record-full.h" and
> 	"linux-record.h".

These two entries are in the same file, so merge them, like:

	* arm-linux-tdep.c: Include "record-full.h" and
	"linux-record.h".
	(struct arm_linux_record_tdep): Declare.
	(arm_canonicalize_syscall): New function.
	(arm_all_but_pc_registers_record): New function.
	(arm_linux_syscall_record): New function.
	(arm_linux_init_abi): Add syscall recording constructs.

> +/* ARM process record-replay constructs; syscall, signal etc.  */
> +
> +struct linux_record_tdep arm_linux_record_tdep;
> +
> +/* arm_canonicalize_syscall maps from the native arm Linux set
> +   of syscall ids into a canonical set of syscall ids used by
> +   process record.  */
> +
> +static enum gdb_syscall
> +arm_canonicalize_syscall (int syscall)
> +{
> +  enum { sys_process_vm_writev = 377 };
> +
> +  if (syscall <=  gdb_sys_sched_getaffinity)
                   ^^

Spurious space.

> +    return syscall;
> +  else if (syscall >= 243 && syscall <= 247 )
> +    return syscall + 2;
> +  else if (syscall >= 248 && syscall <= 253 )
> +    return syscall + 4;
> +
> +  return -1;
> +}
> +

> +}
> +
> +/* Handler for arm system call instruction and recording.  */

Spurious "and" ?  Otherwise I can't parse it.

> +
> +static int
> +arm_linux_syscall_record (struct regcache *regcache, unsigned long svc_number)
> +{
...
> +
> +  ret = record_linux_system_call (syscall_gdb, regcache,
> +                                  &arm_linux_record_tdep);
> +  if (ret)

  if (ret != 0)


> +    return ret;
> +



> +  arm_linux_record_tdep.ioctl_TIOCGSERIAL = 0x541E;
> +  arm_linux_record_tdep.ioctl_TIOCSSERIAL = 0x541F;
...

> +  arm_linux_record_tdep.ioctl_TCGETS2 = 0x802c542a;
> +  arm_linux_record_tdep.ioctl_TCSETS2 = 0x402c542b;

I see a mixup of uppercase and lowercase in these hex constants.
Could you make them all lowercase please?

> +  else
> +    {
> +      arm_record_unsupported_insn(arm_insn_r);

Space before parens.

> +      ret = -1;
> +    }

Otherwise looks good.

-- 
Pedro Alves