Joel Brobecker wrote: >> 2009-09-05 Michael Snyder >> >> * 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. Yes, I'm working with Hui on that. ;-) > 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. Yes, you're right... > 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? Yes, that's historical. The i386 set was implemented first, and then when it was realized that we would need a canonical set, it was just easier to make the existing set into the canonical set. New syscalls that do not belong to i386 get added at the upper end of the canonical set -- which is why there is a magic marker "i386_syscall_max" to separate the original i386 ones from the more general, added ones. I could have commented all of that, but wasn't sure it was important. > 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?) OK, I see your point -- I think I could do something that wasn't too ugly. Get back to you on that. > >> + 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? No. >> @@ -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. Yeah, generally I think it means "come back and look at this later". But as you say, that was there before this patch. > >> - /* XXX RECORD_SEMCTL still not support. */ >> + /* XXX RECORD_SEMCTL still not support. */ > > support -> supported ? OK. >> 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? OK. >> 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. OK. Thanks for the comments. I went ahead and changed syscall to an enum, by the way. New diff attached.