This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
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?