Re: [RFA] cleanup of syscall consts in process record

[Joel Brobecker <brobecker at adacore dot com>]

> 2009-09-05  Michael Snyder  <msnyder@vmware.com>
> 
> 	* 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