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