Re: [RFA] Process record and replay, 5/10

[Eli Zaretskii <eliz at gnu dot org>]

> Date: Thu, 6 Nov 2008 15:48:46 +0800
> From: teawater <teawater@gmail.com>
> 
> +/* Process record and replay target code for GNU/Linux.
> +
> +   Copyright (C) 2008 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 2 of the License, or
> +   (at your option) any later version.  ^^^^^^^^^

I think we need this rephrased to use GPLv3, not v2.

> +/* These macros are the values of the first argument of system call
> +   "sys_ptrace". The values of these macros are gotten from Linux Kernel
> +   source.  */
> +
> +#define RECORD_PTRACE_PEEKTEXT	1
> +#define RECORD_PTRACE_PEEKDATA	2
> +#define RECORD_PTRACE_PEEKUSR	3

Again, shouldn't this kind of data be taken from the syscall database,
rather than being spread over a few source files?  I think having them
in one place will make the code more maintainable.

> +      /* sys_write */
> +    case 4:
> +      /* sys_open */
> +    case 5:
> +      /* sys_close */
> +    case 6:
> +      /* sys_waitpid */
> +    case 7:

Same here.

> +	  yquery (_
> +		  ("The next instruction is syscall exit.  It will make the program exit.  Do you want to stop the program."));

There should be a question mark at the end of the last sentence, not a
period.

> +      /* sys_reboot */
> +    case 88:
> +      {
> +	int q;
> +	target_terminal_ours ();
> +	q =
> +	  yquery (_
> +		  ("The next instruction is syscall reboot.  It will restart the computer.  Do you want to stop the program."));

Same here.

> +	q =
> +	  yquery (_
> +		  ("The next instruction is syscall munmap.  It will free the memory addr = 0x%s len = %d.  It will make record target get error.  Do you want to stop the program."),

And here.

> +	  printf_unfiltered (_
> +			     ("Record: record and reverse target doesn't support ioctl request 0x%08x.\n"),
                                       ^^^^^^^^^^^^^^^^^^
Don't you mean "record and replay"?  (There are a few more of these in
the patch.)

> +      /* sys_ni_syscall */
> +    case 56:
> +      /* sys_setpgid */
> +    case 57:
> +      /* sys_ni_syscall */
> +    case 58:
> +      break;
> +
> +      /* sys_olduname */
> +    case 59:
> +      regcache_raw_read (record_regcache, tdep->arg1, (gdb_byte *) & tmpu32);
> +      if (record_arch_list_add_mem (tmpu32, tdep->size_oldold_utsname))
> +	{
> +	  return (-1);
> +	}
> +      break;
> +
> +      /* sys_umask */
> +    case 60:
> +      /* sys_chroot */
> +    case 61:
> +      break;
> +
> +      /* sys_ustat */
> +    case 62:
> +      regcache_raw_read (record_regcache, tdep->arg2, (gdb_byte *) & tmpu32);
> +      if (record_arch_list_add_mem (tmpu32, tdep->size_ustat))
> +	{
> +	  return (-1);
> +	}
> +      break;

It's a matter of style, I guess, but wouldn't it be better, instead of
endless repetition of almost identical code fragments like the two
above, to put the differing chunks in some data structure and then
just have one instance of the call to regcache_raw_read and
record_arch_list_add_mem, using the data in the data structure?

> +      /* old_select */
> +    case 82:
> +      {
> +	/*
> +	   struct sel_arg_struct {
> +	   unsigned long n;
> +	   fd_set *inp;
> +	   fd_set *outp;
> +	   fd_set *exp;
> +	   struct timeval *tvp;
> +	   };
> +	 */
> +	struct sel_arg_struct
> +	{
> +	  uint32_t n;
> +	  uint32_t inp;
> +	  uint32_t outp;
> +	  uint32_t exp;
> +	  uint32_t tvp;
> +	} sel;

Do we really need the commented-out struct definition?

> +	case RECORD_SYS_GETPEERNAME:
> +	  {
> +	    uint32_t a[3];
> +	    regcache_raw_read (record_regcache, tdep->arg2,
> +			       (gdb_byte *) & tmpu32);
> +	    if (tmpu32)
> +	      {
> +		if (target_read_memory (tmpu32, (gdb_byte *) a, sizeof (a)))
> +		  {
> +		    fprintf_unfiltered (gdb_stdlog,
> +					"Record: read memory addr = 0x%s len = %d error.\n",

Is this a left-over from debugging stage?  If not, why is it needed in
GDB?  (There are few more fprintf_unfiltered's like this one.)