From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15263 invoked by alias); 9 Nov 2008 03:54:35 -0000 Received: (qmail 15120 invoked by uid 22791); 9 Nov 2008 03:54:34 -0000 X-Spam-Check-By: sourceware.org Received: from ti-out-0910.google.com (HELO ti-out-0910.google.com) (209.85.142.190) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sun, 09 Nov 2008 03:53:58 +0000 Received: by ti-out-0910.google.com with SMTP id d10so1085897tib.12 for ; Sat, 08 Nov 2008 19:53:55 -0800 (PST) Received: by 10.110.7.18 with SMTP id 18mr5956701tig.22.1226202835028; Sat, 08 Nov 2008 19:53:55 -0800 (PST) Received: by 10.110.103.3 with HTTP; Sat, 8 Nov 2008 19:53:54 -0800 (PST) Message-ID: Date: Sun, 09 Nov 2008 03: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/msg00157.txt.bz2 Thanks Eli. On Fri, Nov 7, 2008 at 23:09, Eli Zaretskii wrote: >> Date: Thu, 6 Nov 2008 15:48:46 +0800 >> From: teawater >> >> +/* 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?