Re: [RFA/RFC Prec] Add Linux AMD64 process record support second version, (64 bits system call support) 2/3

Michael Snyder
2009-07-18T00:53:25Z

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!