This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Thanks Eli.

On Fri, Nov 7, 2008 at 23:09, Eli Zaretskii <eliz@gnu.org> wrote:
>> 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.

Sorry. I will fix it.

>
>> +/* 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?

>
>> +      /* 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.

I will fix all of them.

>
>> +       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.)
>

I will fix all of them.

>> +      /* 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?


>> +      /* 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?

This part of code is not fit for some arches. I will fix it and remove
this comment.

>
>> +     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?


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]