From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8858 invoked by alias); 7 Nov 2008 15:10:09 -0000 Received: (qmail 8764 invoked by uid 22791); 7 Nov 2008 15:10:08 -0000 X-Spam-Check-By: sourceware.org Received: from mtaout7.012.net.il (HELO mtaout7.012.net.il) (84.95.2.19) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 07 Nov 2008 15:09:26 +0000 Received: from conversion-daemon.i-mtaout7.012.net.il by i-mtaout7.012.net.il (HyperSendmail v2007.08) id <0K9Y00H00Y2YMJ00@i-mtaout7.012.net.il> for gdb-patches@sourceware.org; Fri, 07 Nov 2008 17:11:08 +0200 (IST) Received: from HOME-C4E4A596F7 ([77.126.241.172]) by i-mtaout7.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0K9Y00D9AY6JYWB0@i-mtaout7.012.net.il>; Fri, 07 Nov 2008 17:11:08 +0200 (IST) Date: Fri, 07 Nov 2008 15:10: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/msg00127.txt.bz2 > 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. > +/* 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. > + /* 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. > + 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.) > + /* 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? > + /* 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? > + 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.)