From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29259 invoked by alias); 18 Jul 2009 00:53:25 -0000 Received: (qmail 29247 invoked by uid 22791); 18 Jul 2009 00:53:22 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,J_CHICKENPOX_46 X-Spam-Check-By: sourceware.org Received: from smtp-outbound-2.vmware.com (HELO smtp-outbound-2.vmware.com) (65.115.85.73) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 18 Jul 2009 00:53:14 +0000 Received: from mailhost4.vmware.com (mailhost4.vmware.com [10.16.67.124]) by smtp-outbound-2.vmware.com (Postfix) with ESMTP id 7325711007; Fri, 17 Jul 2009 17:53:12 -0700 (PDT) Received: from [10.20.94.141] (msnyder-server.eng.vmware.com [10.20.94.141]) by mailhost4.vmware.com (Postfix) with ESMTP id 5CEC5C9D94; Fri, 17 Jul 2009 17:53:12 -0700 (PDT) Message-ID: <4A611BB8.2050808@vmware.com> Date: Sat, 18 Jul 2009 03:04:00 -0000 From: Michael Snyder User-Agent: Thunderbird 1.5.0.12 (X11/20080411) MIME-Version: 1.0 To: Hui Zhu CC: gdb-patches ml , mark.kettenis@xs4all.nl Subject: Re: [RFA/RFC Prec] Add Linux AMD64 process record support second version, (64 bits system call support) 2/3 References: <4A5A82F9.5050503@vmware.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: 2009-07/txt/msg00442.txt.bz2 Hui Zhu wrote: > @@ -80,6 +81,133 @@ > #define RECORD_Q_XGETQSTAT (('5' << 8) + 5) > #define RECORD_Q_XGETQUOTA (('3' << 8) + 3) > > +#define OUTPUT_REG(val, num) phex_nz ((val), \ > + TYPE_LENGTH (gdbarch_register_type (get_regcache_arch (regcache), (num)))) > + > +static int > +record_linux_sockaddr (struct regcache *regcache, > + struct linux_record_tdep *tdep, ULONGEST addr, > + ULONGEST len) > +{ > + gdb_byte *a; > + int addrlen; > + struct gdbarch *gdbarch = get_regcache_arch (regcache); > + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > + > + if (!addr) > + return 0; > + > + a = alloca (tdep->size_int); > + > + if (record_arch_list_add_mem ((CORE_ADDR) len, tdep->size_int)) > + return -1; > + > + /* Get the addrlen. */ > + if (target_read_memory ((CORE_ADDR) len, a, tdep->size_int)) > + { > + if (record_debug) > + fprintf_unfiltered (gdb_stdlog, > + "Process record: error reading " > + "memory at addr = 0x%s len = %d.\n", > + phex_nz (len, tdep->size_pointer), > + tdep->size_int); > + return -1; > + } > + addrlen = (int) extract_unsigned_integer(a, tdep->size_int, byte_order); Space between function name and left paren. > + /* msg_iov msg_iovlen */ > + addr = extract_unsigned_integer (a, tdep->size_pointer, byte_order); > + a += tdep->size_pointer; > + if (addr) > + { > + ULONGEST i; > + ULONGEST len = extract_unsigned_integer (a, tdep->size_size_t, > + byte_order); > + gdb_byte *iov = alloca (tdep->size_iovec); > + > + for (i = 0; i < len; i++) > + { > + if (target_read_memory ((CORE_ADDR) addr, iov, tdep->size_iovec)) > + { > + if (record_debug) > + fprintf_unfiltered (gdb_stdlog, > + "Process record: error " > + "reading memory at " > + "addr = 0x%s " > + "len = %d.\n", > + phex_nz (addr,tdep->size_pointer), > + tdep->size_iovec); > + return -1; > + } > + if (record_arch_list_add_mem ((CORE_ADDR) extract_unsigned_integer > + (iov, tdep->size_pointer, > + byte_order), > + (int) extract_unsigned_integer > + (iov + tdep->size_pointer, > + tdep->size_size_t, > + byte_order))) This statement is so ugly and badly indented. It would be great if you could just use a couple of temporary variables, one CORE_ADDR and one int, and break up the line. Call extract_unsigned_integer first, and then record_arch_list_add_mem. > + return -1; > + addr += tdep->size_iovec; > + } > + } > + a += tdep->size_size_t; > + > + /* msg_control msg_controllen */ > + addr = extract_unsigned_integer (a, tdep->size_pointer, byte_order); > + a += tdep->size_pointer; > + if (record_arch_list_add_mem ((CORE_ADDR) addr, > + (int) extract_unsigned_integer > + (a, tdep->size_size_t, byte_order))) Same here, maybe use the same temporary variable. > + case 514: > + regcache_raw_read_unsigned (regcache, tdep->arg5, &tmpulongest); > + if (tmpulongest) > + { > + ULONGEST optvalp; > + gdb_byte *optlenp = alloca (tdep->size_int); > + if (target_read_memory ((CORE_ADDR) tmpulongest, optlenp, > + tdep->size_int)) > + { > + if (record_debug) > + fprintf_unfiltered (gdb_stdlog, > + "Process record: error reading " > + "memory at addr = 0x%s " > + "len = %d.\n", > + OUTPUT_REG (tmpulongest, tdep->arg5), > + tdep->size_int); > + return -1; > + } > + regcache_raw_read_unsigned (regcache, tdep->arg4, &optvalp); > + if (record_arch_list_add_mem ((CORE_ADDR) optvalp, > + (int) extract_signed_integer > + (optlenp, tdep->size_int, > + byte_order))) Another great place for a temporary variable, just to avoid having to break up such a long line so awkwardly. > + tmpulongest += tdep->size_ulong; > + if (target_read_memory ((CORE_ADDR) tmpulongest, a, > + tdep->size_ulong * 2)) > + { > + if (record_debug) > + fprintf_unfiltered (gdb_stdlog, > + "Process record: error reading " > + "memory at addr = 0x%s len = %d.\n", > + OUTPUT_REG (tmpulongest, tdep->arg2), > + tdep->size_ulong * 2); > + return -1; > + } > + if (record_linux_sockaddr (regcache, tdep, > + extract_unsigned_integer > + (a, tdep->size_ulong, byte_order), > + extract_unsigned_integer > + (a + tdep->size_ulong, > + tdep->size_ulong, byte_order))) ... and again, same thing. > + return -1; > + } > + } > + break; > > - regcache_raw_read (regcache, tdep->arg2, > - (gdb_byte *) & tmpu32); > - if (tmpu32) > - { > - if (target_read_memory (tmpu32, (gdb_byte *) a, sizeof (a))) > - { > - if (record_debug) > - fprintf_unfiltered (gdb_stdlog, > - "Process record: error reading " > - "memory at addr = %s len = %lu.\n", > - paddress (gdbarch, tmpu32), > - (unsigned long)sizeof (a)); > - return -1; > - } > - if (a[4]) > - { > - if (target_read_memory > - (a[4], (gdb_byte *) & av, sizeof (av))) How about: if (target_read_memory (a[4], gdb_byte *) & av, sizeof (av))) > + case RECORD_SYS_SOCKETPAIR: > + { > + gdb_byte *a = alloca (tdep->size_ulong); > + regcache_raw_read_unsigned (regcache, tdep->arg2, > + &tmpulongest); > + if (tmpulongest) > + { > + tmpulongest += tdep->size_ulong * 3; > + if (target_read_memory ((CORE_ADDR) tmpulongest, a, > + tdep->size_ulong)) > + { > + if (record_debug) > + fprintf_unfiltered (gdb_stdlog, > + "Process record: error reading " > + "memory at addr = 0x%s len = %d.\n", > + OUTPUT_REG (tmpulongest, tdep->arg2), > + tdep->size_ulong); > + return -1; > + } > + if (record_arch_list_add_mem ((CORE_ADDR) > extract_unsigned_integer > + (a, > + tdep->size_ulong, > + byte_order), > + tdep->size_int)) And another great place for a temp variable. > + tmpulongest += tdep->size_ulong * 4; > + if (target_read_memory ((CORE_ADDR) tmpulongest, a, > + tdep->size_ulong * 2)) > + { > + if (record_debug) > + fprintf_unfiltered (gdb_stdlog, > + "Process record: error reading " > + "memory at addr = 0x%s len = %d.\n", > + OUTPUT_REG (tmpulongest, tdep->arg2), > + tdep->size_ulong * 2); > + return -1; > + } > + if (record_linux_sockaddr (regcache, tdep, > + extract_unsigned_integer > + (a, tdep->size_ulong, byte_order), > + extract_unsigned_integer > + (a + tdep->size_ulong, > + tdep->size_ulong, byte_order))) > + return -1; > + } > + case RECORD_SYS_RECV: And again... > + regcache_raw_read_unsigned (regcache, tdep->arg2, > + &tmpulongest); > + if (tmpulongest) > + { > + gdb_byte *a = alloca (tdep->size_ulong * 2); > + > + tmpulongest += tdep->size_ulong; > + if (target_read_memory ((CORE_ADDR) tmpulongest, a, > + tdep->size_ulong)) > + { > + if (record_debug) > + fprintf_unfiltered (gdb_stdlog, > + "Process record: error reading " > + "memory at addr = 0x%s len = %d.\n", > + OUTPUT_REG (tmpulongest, tdep->arg2), > + tdep->size_ulong); > + return -1; > + } > + tmpulongest = extract_unsigned_integer (a, tdep->size_ulong, > + byte_order); > + if (tmpulongest) > + { > + a += tdep->size_ulong; > + if (record_arch_list_add_mem > + ((CORE_ADDR) tmpulongest, > + (int) extract_unsigned_integer (a, tdep->size_ulong, > + byte_order))) > + return -1; > + } > + } > + break; > + case RECORD_SYS_SHUTDOWN: And again.... > + case RECORD_SYS_SETSOCKOPT: > + break; > + case RECORD_SYS_GETSOCKOPT: > + { > + gdb_byte *a = alloca (tdep->size_ulong * 2); > + gdb_byte *av = alloca (tdep->size_int); > + > + regcache_raw_read_unsigned (regcache, tdep->arg2, > + &tmpulongest); > + if (tmpulongest) > + { > + tmpulongest += tdep->size_ulong * 3; > + if (target_read_memory ((CORE_ADDR) tmpulongest, a, > + tdep->size_ulong * 2)) > + { > + if (record_debug) > + fprintf_unfiltered (gdb_stdlog, > + "Process record: error reading " > + "memory at addr = 0x%s len = %d.\n", > + OUTPUT_REG (tmpulongest, tdep->arg2), > + tdep->size_ulong * 2); > + return -1; > + } > + tmpulongest = extract_unsigned_integer (a + tdep->size_ulong, > + tdep->size_ulong, > + byte_order); > + if (tmpulongest) > + { > + if (target_read_memory ((CORE_ADDR) tmpulongest, av, > + tdep->size_int)) > + { > + if (record_debug) > + fprintf_unfiltered (gdb_stdlog, > + "Process record: error reading " > + "memory at addr = 0x%s " > + "len = %d.\n", > + phex_nz (tmpulongest, > + tdep->size_ulong), > + tdep->size_int); > + return -1; > + } > + if (record_arch_list_add_mem > + ((CORE_ADDR) extract_unsigned_integer > + (a, tdep->size_ulong, byte_order), > + (int) extract_unsigned_integer (av, tdep->size_int, > + byte_order))) And again... > + return -1; > + a += tdep->size_ulong; > + if (record_arch_list_add_mem > + ((CORE_ADDR) extract_unsigned_integer > + (a, tdep->size_ulong, byte_order), > + tdep->size_int)) And again... > + return -1; > + } > + } > + } > + break; > + case RECORD_SYS_SENDMSG: > + break; > + case RECORD_SYS_RECVMSG: > + { > + gdb_byte *a = alloca (tdep->size_ulong); > + > + regcache_raw_read_unsigned (regcache, tdep->arg2, > + &tmpulongest); > + if (tmpulongest) > + { > + tmpulongest += tdep->size_ulong; > + if (target_read_memory ((CORE_ADDR) tmpulongest, a, > + tdep->size_ulong)) > + { > + if (record_debug) > + fprintf_unfiltered (gdb_stdlog, > + "Process record: error reading " > + "memory at addr = 0x%s len = %d.\n", > + OUTPUT_REG (tmpulongest, tdep->arg2), > + tdep->size_ulong); > + return -1; > + } > + tmpulongest = extract_unsigned_integer (a, tdep->size_ulong, > + byte_order); > + if (record_linux_msghdr (regcache, tdep, > + extract_unsigned_integer > + (a, tdep->size_ulong, byte_order))) And again... > @@ -1005,47 +1231,121 @@ record_linux_system_call (int num, struc > > /* sys_sysinfo */ > case 116: > - regcache_raw_read (regcache, tdep->arg1, (gdb_byte *) & tmpu32); > - if (record_arch_list_add_mem (tmpu32, tdep->size_sysinfo)) > - return -1; > + regcache_raw_read_unsigned (regcache, tdep->arg1, &tmpulongest); > + if (record_arch_list_add_mem ((CORE_ADDR) tmpulongest, > + tdep->size_sysinfo)) > + return -1; > + break; > + > + /* sys_shmget */ > + case 520: > + /* sys_semget */ > + case 523: > + /* sys_semop */ > + case 524: > + /* sys_msgget */ > + case 528: > + /* sys_shmdt */ > + /* XXX maybe need do some record works wiht sys_shmdt. */ "with" > + ULONGEST vec, vlen; > + > + regcache_raw_read_unsigned (regcache, tdep->arg2, &vec); > + if (vec) > + { > + gdb_byte *iov = alloca (tdep->size_iovec); > + > + regcache_raw_read_unsigned (regcache, tdep->arg3, &vlen); > + for (tmpulongest = 0; tmpulongest < vlen; tmpulongest++) > + { > + if (target_read_memory ((CORE_ADDR) vec, iov, > + tdep->size_iovec)) > + { > + if (record_debug) > + fprintf_unfiltered (gdb_stdlog, > + "Process record: error reading " > + "memory at addr = 0x%s len = %d.\n", > + OUTPUT_REG (vec, tdep->arg2), > + tdep->size_iovec); > + return -1; > + } > + if (record_arch_list_add_mem > + ((CORE_ADDR) extract_unsigned_integer > + (iov, tdep->size_pointer, byte_order), > + (int) extract_unsigned_integer > + (iov + tdep->size_pointer, tdep->size_size_t, > + byte_order))) And same thing... > + regcache_raw_read_unsigned (regcache, tdep->arg3, &tmpulongest); > + if (tmpulongest) > + { > + ULONGEST nr, i; > + gdb_byte *iocbp; > + > + regcache_raw_read_unsigned (regcache, tdep->arg2, &nr); > + iocbp = alloca (nr * tdep->size_pointer); > + if (target_read_memory ((CORE_ADDR) tmpulongest, iocbp, > + nr * tdep->size_pointer)) > + { > + if (record_debug) > + fprintf_unfiltered (gdb_stdlog, > + "Process record: error reading memory " > + "at addr = 0x%s len = %u.\n", > + OUTPUT_REG (tmpulongest, tdep->arg2), > + (int) (nr * tdep->size_pointer)); > + return -1; > + } > + for (i = 0; i < nr; i++) > + { > + if (record_arch_list_add_mem > + ((CORE_ADDR) extract_unsigned_integer (iocbp, > + tdep->size_pointer, > + byte_order), > + tdep->size_iocb)) And again... > case 319: > - regcache_raw_read (regcache, tdep->arg2, (gdb_byte *) & tmpu32); > - if (tmpu32) > - { > - int32_t maxevents; > - regcache_raw_read (regcache, tdep->arg3, > - (gdb_byte *) & maxevents); > - if (record_arch_list_add_mem > - (tmpu32, maxevents * tdep->size_epoll_event)) > - return -1; > - } > + regcache_raw_read_unsigned (regcache, tdep->arg2, &tmpulongest); > + if (tmpulongest) > + { > + ULONGEST maxevents; > + regcache_raw_read_unsigned (regcache, tdep->arg3, &maxevents); > + if (record_arch_list_add_mem ((CORE_ADDR) tmpulongest, > + (int) maxevents * > tdep->size_epoll_event)) I think you don't need the (int) cast here, and if you remove it, the line will be less than 80 chars. Aside from those comments, this patch looks great!