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

[teawater <teawater@gmail.com>]

Thanks Eli.

On Mon, Nov 10, 2008 at 03:58, Eli Zaretskii <eliz@gnu.org> wrote:
>> 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.
>

I will add a record_debug in any place like there.