From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13423 invoked by alias); 11 Nov 2008 05:50:33 -0000 Received: (qmail 13327 invoked by uid 22791); 11 Nov 2008 05:50:32 -0000 X-Spam-Check-By: sourceware.org Received: from ti-out-0910.google.com (HELO ti-out-0910.google.com) (209.85.142.191) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 11 Nov 2008 05:49:28 +0000 Received: by ti-out-0910.google.com with SMTP id d10so1814422tib.12 for ; Mon, 10 Nov 2008 21:49:25 -0800 (PST) Received: by 10.110.47.17 with SMTP id u17mr8863762tiu.19.1226382565537; Mon, 10 Nov 2008 21:49:25 -0800 (PST) Received: by 10.110.103.3 with HTTP; Mon, 10 Nov 2008 21:49:25 -0800 (PST) Message-ID: Date: Tue, 11 Nov 2008 18:54:00 -0000 From: teawater To: "Eli Zaretskii" Subject: Re: [RFA] Process record and replay, 5/10 Cc: gdb-patches@sourceware.org In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline 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/msg00210.txt.bz2 Thanks Eli. On Mon, Nov 10, 2008 at 03:58, Eli Zaretskii wrote: >> 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. > I will add a record_debug in any place like there.