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

[Eli Zaretskii <eliz@gnu.org>]

> Date: Sun, 9 Nov 2008 11:53:54 +0800
> From: teawater <teawater@gmail.com>
> Cc: gdb-patches@sourceware.org
> 
> >> +/* 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.
> 
> What about I make a special .h file for each of them?

I was referring to the syscall database that is part of the "catch
syscalls" patch discussed elsewhere in this list.  I think we should
have all the data about Linux system calls in the same place.

> >> +      /* 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?
> >
> 
> Sorry,  I am not very clear your mean. I am not native speaker. :(
> 
> Do you mean is put this code to a function that has a argv is tdep?

No, I mean to define a data structure, like this:

  struct syscall_entry {
    int num;
    size_t size;
  } syscall_data[] = {
    ...
    { 56, 0 },
    { 57, 0 },
    { 58, 0 },
    { 59, tdep->size_oldold_utsname },
    ...
  };

and then use it like this:

    if (syscall_data[i].size)
      {
        regcache_raw_read (record_regcache, tdep->arg1, (gdb_byte *) & tmpu32);
	if (record_arch_list_add_mem (tmpu32, syscall_data[i].size))
	  {
      	    return (-1);
    	  }
      }
      break;

You can then have only one (or maybe few different) code fragments,
and the rest will be recorded in the data structure.

> >> +     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.)
> >
> I want let user know what happen when got a error. What do you think about it?

I think we should at least have a user option to turn it on and off.