From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30884 invoked by alias); 9 Nov 2008 19:58:58 -0000 Received: (qmail 30756 invoked by uid 22791); 9 Nov 2008 19:58:57 -0000 X-Spam-Check-By: sourceware.org Received: from mtaout4.012.net.il (HELO mtaout4.012.net.il) (84.95.2.10) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sun, 09 Nov 2008 19:58:15 +0000 Received: from conversion-daemon.i_mtaout4.012.net.il by i_mtaout4.012.net.il (HyperSendmail v2004.12) id <0KA3006000TV6S00@i_mtaout4.012.net.il> for gdb-patches@sourceware.org; Sun, 09 Nov 2008 21:59:59 +0200 (IST) Received: from HOME-C4E4A596F7 ([77.126.241.172]) by i_mtaout4.012.net.il (HyperSendmail v2004.12) with ESMTPA id <0KA300KA00VX9SD1@i_mtaout4.012.net.il>; Sun, 09 Nov 2008 21:59:58 +0200 (IST) Date: Sun, 09 Nov 2008 19:58:00 -0000 From: Eli Zaretskii Subject: Re: [RFA] Process record and replay, 5/10 In-reply-to: X-012-Sender: halo1@inter.net.il To: teawater Cc: gdb-patches@sourceware.org Reply-to: Eli Zaretskii Message-id: References: X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-11/txt/msg00162.txt.bz2 > Date: Sun, 9 Nov 2008 11:53:54 +0800 > From: teawater > 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.