From: Michael Snyder <msnyder@vmware.com>
To: Hui Zhu <teawater@gmail.com>
Cc: gdb-patches ml <gdb-patches@sourceware.org>, mark.kettenis@xs4all.nl
Subject: Re: [RFA/RFC Prec] Add Linux AMD64 process record support second version, (64 bits system call support) 2/3
Date: Sat, 18 Jul 2009 03:04:00 -0000 [thread overview]
Message-ID: <4A611BB8.2050808@vmware.com> (raw)
In-Reply-To: <daef60380907170147m219fffabtca0e997a69565852@mail.gmail.com>
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!
next prev parent reply other threads:[~2009-07-18 0:53 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-07-07 2:40 Hui Zhu
2009-07-13 2:26 ` Michael Snyder
2009-07-17 12:03 ` Hui Zhu
2009-07-18 3:04 ` Michael Snyder [this message]
2009-07-19 17:39 ` Hui Zhu
2009-07-26 7:46 ` Michael Snyder
2009-07-28 1:44 ` Hui Zhu
2009-08-03 5:40 ` Hui Zhu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4A611BB8.2050808@vmware.com \
--to=msnyder@vmware.com \
--cc=gdb-patches@sourceware.org \
--cc=mark.kettenis@xs4all.nl \
--cc=teawater@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox