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

[Mark Kettenis <mark.kettenis@xs4all.nl>]

> Date: Tue, 08 Sep 2009 12:06:27 -0700
> From: Michael Snyder <msnyder@vmware.com>
> 
> Joel Brobecker wrote:
> >> 2009-09-08  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_canonicalize_syscall): New function,
> >> 	trivially translate from native i386 Linux syscalls to gdb syscalls.
> >> 	(i386_linux_intx80_sysenter_record): 
> > 
> > Nice! I really like this version much better. The approach you took
> > with i386 made me wonder whether we really need the amd64_syscall enum
> > at all - we could have used a plain int as the argument to
> > amd64_canonicalize_syscall, and use plain numbers there, rather than
> > having an enum that's only used there. I don't mind, though, so
> > don't worry about it unless you agree as well.
> 
> Nah, the idea was to get rid of magic numbers,
> plus it makes the code more readable.  Self-documenting.
> 
> 
> > Note that this should also fix the issue that Hui reported about
> > building on cygwin with --enable-64-bit-bfd. So I'll remove Hui's
> > patch from my list.
> 
> Yes, I broke down and included Hui's patch in this one.
> 
> > Just one comment:
> > 
> >> +static enum gdb_syscall
> >> +i386_canonicalize_syscall (int syscall)
> >> +{
> >> +  enum { i386_syscall_max = 499 };
> >> +
> >> +  if (syscall <= i386_syscall_max)
> >> +    return syscall;
> > 
> > I thought that we should incorporate Mark's suggestion of checking
> > syscall against negative values. But I now realize that if syscall
> > is negative, we'll return a value that's equivalent to returning -1.
> > And the check against negative values in i386_linux_intx80_sysenter_record
> > should then catch it.
> 
> Correct.
> 
> So this is approved, then?

Fine with me.