From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20878 invoked by alias); 8 Sep 2009 16:49:59 -0000 Received: (qmail 20791 invoked by uid 22791); 8 Sep 2009 16:49:56 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 08 Sep 2009 16:49:43 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 84A132BAB37; Tue, 8 Sep 2009 12:49:41 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id jrKhK++QE+WA; Tue, 8 Sep 2009 12:49:41 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 1AF792BAB28; Tue, 8 Sep 2009 12:49:41 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id DB03CF589B; Tue, 8 Sep 2009 09:49:34 -0700 (PDT) Date: Tue, 08 Sep 2009 16:49:00 -0000 From: Joel Brobecker To: Michael Snyder Cc: "gdb-patches@sourceware.org" , Hui Zhu , Mark Kettenis Subject: Re: [RFA] cleanup of syscall consts in process record Message-ID: <20090908164934.GP30677@adacore.com> References: <4AA2D95B.4090904@vmware.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4AA2D95B.4090904@vmware.com> User-Agent: Mutt/1.5.18 (2008-05-17) 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: 2009-09/txt/msg00197.txt.bz2 > 2009-09-05 Michael Snyder > > * amd64-linux-tdep.h (enum amd64_syscall): New enum consts, > to replace literal consts used in amd64-linux-tdep.c > * linux-record.h (enum gdb_syscall): New enum consts, to replace > literal consts used in amd64-linux-tdep.c and linux-record.c. > * amd64-linux-tdep.c (amd64_canonicalize_syscall): New function, > translate from native amd64 linux syscall id to internal gdb id. > (amd64_linux_syscall_record): Switch statement abstracted out > and replaced with a call to amd64_canonicalize_syscall. > * linux-record.c (record_linux_system_call): Replace literal > consts with enum consts. > * i386-linux-tdep.c (i386_linux_intx80_sysenter_record): > Replace magic number 499 with enum const. First of all, a big THANK YOU for adding comments to the functions that you are adding. I had a harder time that I needed to when looking at record.c just because I couldn't see any documentation for some of the functions that were called in the code I looked at. Overall, I don't see any problem with the patch. Just a few comments... > +enum gdb_syscall { > + gdb_sys_restart_syscall = 0, [...] > + > + i386_syscall_max = 499, /* Upper limit used by > + i386_linux_intx80_sysenter_record. */ It seems strange to have this enum that's specific to i386 in the middle of a type that is target independent. Just some thoughts on this (these are not objections): I didn't see an enum list defined for i386, just amd64. Does it mean that i386 has syscall ids that are identical to the ones used in enum gdb_syscall? I wouldn't find this very elegant as it makes one architecture different from the others in terms of implementation. If we had an enum type specific to i386, we could then put i386_syscall_max there... Otherwise, a comment explaining that i386-linux syscall numbers are identical to the ones defined in enum gdb_syscall, somewhere, would be very useful (perhaps at the location where we would have converted the target syscall ID to the GDB syscall ID) - i386_linux_intx80_sysenter_record?) > + if (syscall_gdb < 0) > + { > printf_unfiltered (_("Process record and replay target doesn't " > - "support syscall number %d\n"), (int) tmpulongest); > + "support syscall number %d\n"), > + (int) syscall_native); I don't think the cast is necessary, here, is it? > @@ -496,7 +440,7 @@ record_linux_system_call (int num, struc > { > regcache_raw_read_unsigned (regcache, tdep->arg3, > &tmpulongest); > - /* This syscall affect a char size memory. */ > + /* This syscall affect a char size memory. */ ^^^^^^ ^^^^^^^^^ affects char-size (hyphen)? > + case gdb_sys_fcntl: > /* XXX */ Not sure what the "XXX" convention means... FIXME? In any case, hardly relevant to this patch, so just being curious. > - /* XXX RECORD_SEMCTL still not support. */ > + /* XXX RECORD_SEMCTL still not support. */ support -> supported ? > printf_unfiltered (_("Process record and replay target doesn't " > "support ipc number %d\n"), (int) tmpulongest); While we're looking at this: Would you mind getting rid of the cast above by using pulongest? > printf_unfiltered (_("Process record and replay target doesn't " > - "support syscall number %u\n"), num); > + "support syscall number %u\n"), syscall); syscall is an int, so I think you also need to change the %u. -- Joel