From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18926 invoked by alias); 17 Apr 2008 15:29:11 -0000 Received: (qmail 18916 invoked by uid 22791); 17 Apr 2008 15:29:10 -0000 X-Spam-Check-By: sourceware.org Received: from igw3.br.ibm.com (HELO igw3.br.ibm.com) (32.104.18.26) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 17 Apr 2008 15:28:40 +0000 Received: from mailhub3.br.ibm.com (unknown [9.18.232.110]) by igw3.br.ibm.com (Postfix) with ESMTP id 646C83901D4 for ; Thu, 17 Apr 2008 12:15:24 -0300 (BRST) Received: from d24av02.br.ibm.com (d24av02.br.ibm.com [9.18.232.47]) by mailhub3.br.ibm.com (8.13.8/8.13.8/NCO v8.7) with ESMTP id m3HFSZXe4505624 for ; Thu, 17 Apr 2008 12:28:35 -0300 Received: from d24av02.br.ibm.com (loopback [127.0.0.1]) by d24av02.br.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id m3HFSYPA031824 for ; Thu, 17 Apr 2008 12:28:34 -0300 Received: from [9.18.199.219] ([9.18.199.219]) by d24av02.br.ibm.com (8.12.11.20060308/8.12.11) with ESMTP id m3HFSYi4031813; Thu, 17 Apr 2008 12:28:34 -0300 Subject: Re: GDB record patch 0.1.3 for GDB-6.8 release (It make I386-Linux GDB support Reversible Debugging) From: Thiago Jung Bauermann To: Hui Zhu Cc: gdb-patches@sourceware.org In-Reply-To: <4805A420.5060807@windriver.com> References: <4805A420.5060807@windriver.com> Content-Type: multipart/mixed; boundary="=-hRwD1ZLiBCLPjzb2PkC8" Date: Thu, 17 Apr 2008 15:51:00 -0000 Message-Id: <1208446046.5808.51.camel@localhost.localdomain> Mime-Version: 1.0 X-Mailer: Evolution 2.12.3 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-04/txt/msg00338.txt.bz2 --=-hRwD1ZLiBCLPjzb2PkC8 Content-Type: text/plain Content-Transfer-Encoding: 7bit Content-length: 2429 Hi, On Wed, 2008-04-16 at 15:00 +0800, Hui Zhu wrote: > GDB record patch make GDB support Reversible Debugging. > It make GDB disassemble the instruction that will be executed to get which memory and register will be changed and record them to record all program running message. Through these on the use of this information to achieve the implementation of the GDB Reversible Debugging function. Thanks for the patch! Do you have already the FSF copyright assignment in place? You will need it for a significant patch like this to get in. I didn't have a detailed look at it yet, so these are general comments and initial issues that in my opinion need to be adressed before the meat of the patch gets discussed. First, you need to make your code conform to the GNU Coding Standard (http://www.gnu.org/prep/standards/html_node/Writing-C.html) so that it blends uniformly with the rest of the GDB code. Especially regarding indentation (use two spaces per level) and curly bracket positioning. Also, there's almost no comments in the code, which makes it hard to to review the patch for inclusion (that's basically why I didn't look at it in more detail yet). There should be a comment for each function you add to GDB (unless it's very small and obvious, I think) describing its purpose, arguments and return value. And a comment for each structure you add, describing its purpose and that of its elements. Tricky or subtle parts of the code also benefit from some comments explaining why they are needed, or the assumptions they make. Also, in the beginning of gdb/record.c there should be a big comment explaining the general idea and workings of the record mechanism. This will also help us when reviewing the patch, and discuss the approach and its pros and cons. Several places in your patch use current_gdbarch. GDB is currently moving away from it, and there are folks actively working on removing it entirely. The reason is that a single program could have more than one architecture (see the Cell processor and its PPU and SPUs for example). I think you will need help to decide where to get a valid gdbarch from, depending on the context that you need it. I hope this is enough to get the ball rolling. :-) I attached some comments on specific parts of the patch (man, I think I need to start using mutt for reading mailing lists). -- []'s Thiago Jung Bauermann Software Engineer IBM Linux Technology Center --=-hRwD1ZLiBCLPjzb2PkC8 Content-Disposition: attachment; filename=record_patch_comments.txt Content-Type: text/plain; name=record_patch_comments.txt; charset=UTF-8 Content-Transfer-Encoding: 7bit Content-length: 2360 > Signed-off-by: Hui Zhu I believe this has no meaning outside of the Linux kernel community, so there's no need to sign off here (this purpose is filled by the FSF copyright assignment paperwork). > +/*teawater rec begin----------------------------------------------------------*/ > +/*teawater rec end------------------------------------------------------------*/ All these markers all have to go. They also make patch reviewing more cumbersome, IMHO. > +#define RECORD_SIZE__old_kernel_stat 32 > +#define RECORD_SIZE_tms 16 > +#define RECORD_SIZE_loff_t 8 > +#define RECORD_SYS_SOCKET 1 > +#define RECORD_SYS_BIND 2 > +#define RECORD_SYS_CONNECT 3 > +#define RECORD_SEMOP 1 > +#define RECORD_SEMGET 2 > +#define RECORD_SEMCTL 3 > +#define RECORD_Q_GETFMT 0x800004 > +#define RECORD_Q_GETINFO 0x800005 > +#define RECORD_Q_GETQUOTA 0x800007 > +#define RECORD_Q_XGETQSTAT (('5'<<8)+(5)) > +#define RECORD_Q_XGETQUOTA (('3'<<8)+(3)) Comments should be added explaining the purpose of these constants, and where the numbers come from. > +#if 0 > + if (override) { > + /* *addr += ((uint32_t)read_register (override)) << 4; */ > + printf_unfiltered (_("record: cann't get the value of the segment register.\n")); > + return (-1); > + } > +#endif This should be removed. There are two other snippets of code which are #ifed out and should go too. > +/*teawater rec begin----------------------------------------------------------*/ > + int (*intx80_record) (void); > + int (*sysenter_record) (void); > +/*teawater rec end------------------------------------------------------------*/ The names above are specific to the x86 architecture. tdep members should have a name which is meaningful accross different architectures. > SHELL = @SHELL@ > -EXEEXT = @EXEEXT@ > +#teawater rec begin------------------------------------------------------------- > +EXEEXT = @EXEEXT@record > +#teawater rec end--------------------------------------------------------------- This should be removed. > +typedef struct record_s { > + struct record_s *prev; > + struct record_s *next; > + int type; /* 1 reg 2 mem 0 end */ > + union { > + /* reg */ > + record_reg_t reg; > + /* mem */ > + record_mem_t mem; > + /* end */ > + int need_dasm; > + } u; > +} record_t; I suggest changing type to an enum. --=-hRwD1ZLiBCLPjzb2PkC8--