This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA] Linux Checkpoint/Restart, take 2


On Mon, Dec 19, 2005 at 07:18:06PM -0800, Michael Snyder wrote:
> OK, superceding the previous submission, this patch includes
> suggested mods from Eli and Tom Tromey.  Fixed comment formatting,
> tweaked documentation, added NEWS and gdbint.texinfo.
> 
> I also removed the "restart-auto-finish" feature, which is perhaps
> not ready for prime time.  Can always add it back later.

Sorry I didn't get a chance to look at this until now; I was too
intimidated by it.  I still am; it's huge, I can't review it all in the
time I've got, so this look is somewhat quick.

General comments:

You only enable this for i386-linux.  Please enable it everywhere that
the current implementation is expected to work, i.e. everywhere that uses
linux-nat.c.  In fact you'd better do that, because otherwise you've
broken the build of all other native GNU/Linux targets :-)

The implementation is almost entirely contained in linux-nat.c and
linux-fork.c.  Maybe we should discuss the target vector interface to
this before it goes in?  For instance, there's no reason a Linux-hosted
gdbserver shouldn't be able to implement exactly the same thing.

I totally don't understand what the clobber_regs bits are for. You're
using fork as a backend; each saved fork had better have both its own
registers, right?  Is it just a GDB internal bookkeeping thing?  If so,
why do you need to do it for saved forks and not for the existing
follow-fork bits?

I was somewhat amazed to find out that the same is not true of file
descriptor offsets.  In fact I had to go write a test program for it.

Could you make a pass through comment formatting, please?  Not a lot, but
enough variety that it jumped out at me, e.g. */ shouldn't generally be
on its own line, missing final period and two spaces.  Also some
unnecessary braces for single statements.

There's a bunch of FIXMEs.  I'm not really happy about committing a 
patch this big with FIXMEs in it; can we try to straighten them out
first?

All the calls to error, and most of the other output calls, should be
_("") wrapped in our current world order.

We know this is going to fall down badly with threads.  Can we do
something friendly about that?  Maybe you already do, I didn't look too
hard.

> Index: NEWS
> ===================================================================
> RCS file: /cvs/src/src/gdb/NEWS,v
> retrieving revision 1.181
> diff -p -r1.181 NEWS
> *** NEWS	8 Dec 2005 19:13:00 -0000	1.181
> --- NEWS	20 Dec 2005 03:05:22 -0000
> ***************
> *** 8,13 ****
> --- 8,42 ----
>   init-if-undefined		Initialize a convenience variable, but
>   				only if it doesn't already have a value.
>   
> + The following commands are presently only implemented for native linux:

"GNU/Linux" please.

> + 
> + checkpoint			Save a snapshot of the program state.
> + 
> + restart	<n>			Return the program state to a 
> + 				previously saved state.
> + 
> + info checkpoints		List currently saved checkpoints.
> + 
> + delete-checkpoint <n>		Delete a previously saved checkpoint.
> + 
> + set|show detach-on-fork		Tell gdb whether to detach from a newly
> + 				forked process, or to keep debugging it.
> + 
> + info forks			List forks of the user program that
> + 				are available to be debugged.

Why have we got both "info checkpoints" and "info forks"?  Right now
they're aliases to each other and some of the fork commands redirect
you to info checkpoints.

> + 
> + fork <n>			Switch to debugging one of several
> + 				forks of the user program that are
> + 				available to be debugged.

How do you feel about calling this "switch-fork"?  If I see a debugger
command named "fork", my first reaction is going to be that it's an
active command (creates a fork) instead of a passive one (focuses our
attention on a different one).

> + 
> + delete-fork <n>			Delete a fork from the list of forks
> + 				that are available to be debugged (and
> + 				kill the forked process).
> + 
> + detach-fork <n>			Delete a fork from the list of forks
> + 				that are available to be debugged (and
> + 				allow the process to continue).
> + 
>   * New architecture
>   
>   Morpho Technologies ms2		ms1-elf

> Index: linux-fork.c

> + static void
> + info_forks_command (char *arg, int from_tty)
> + {
> +   struct frame_info *cur_frame;
> +   struct symtab_and_line sal;
> +   struct symtab *cur_symtab;
> +   struct fork_info *fp;
> +   int cur_line;
> +   ULONGEST pc;
> + 
> +   for (fp = fork_list; fp; fp = fp->next)
> +     {
> +       if (ptid_equal (fp->ptid, inferior_ptid))
> + 	{
> + 	  printf_filtered ("* ");
> + 	  pc = read_pc ();
> + 	}
> +       else
> + 	{
> + 	  printf_filtered ("  ");
> + 	  regcache_raw_read_unsigned (fp->savedregs, PC_REGNUM, &pc);
> + 	}

Getting at the PC this way is probably going to bite you; the two that
jump out at me are you've bypassed ADDR_BITS_REMOVE, and you're forcing
unsigned (which is wrong for MIPS and might result in some strange
looking PCs).  No, I don't have a better idea.

> + static void
> + checkpoint_command (char *args, int from_tty)
> + {
> +   struct target_waitstatus last_target_waitstatus;
> +   ptid_t last_target_ptid;
> +   struct value *fork_fn = NULL, *ret;
> +   struct fork_info *fp;
> +   pid_t retpid;
> +   int save_detach_fork;
> +   long i;
> + 
> +   /* Make the inferior fork, record its (and gdb's) state.  */
> + 
> +   if (lookup_minimal_symbol ("fork", NULL, NULL) != NULL)
> +     fork_fn = find_function_in_inferior ("fork");
> +   if (!fork_fn)
> +     if (lookup_minimal_symbol ("_fork", NULL, NULL) != NULL)
> +       fork_fn = find_function_in_inferior ("fork");
> +   if (!fork_fn)
> +     error ("checkpoint: can't find fork function in inferior.");
> + 
> +   ret = value_from_longest (builtin_type_int, 0);
> +   save_detach_fork = detach_fork;
> +   detach_fork = 0;
> +   ret = call_function_by_hand (fork_fn, 0, &ret);
> +   detach_fork = save_detach_fork;
> +   if (!ret)	/* Probably can't happen.  */
> +     error ("checkpoint: call_function_by_hand returned null.");

You've gotta use cleanups for this sort of thing.  e.g. what
if the inferior takes a signal during the call_function_by_hand?  Very
easy to arrange; we're coming from a user prompt here, and
call_function_by_hand is a stopped -> running transition.


> + 
> +   retpid = value_as_long (ret);
> +   get_last_target_status (&last_target_ptid, &last_target_waitstatus);
> +   if (from_tty)
> +     {
> +       int parent_pid;
> + 
> +       printf_filtered ("checkpoint: fork returned %ld.\n", (long) retpid);
> +       parent_pid = ptid_get_lwp (last_target_ptid);
> +       if (parent_pid == 0)
> + 	parent_pid = ptid_get_pid (last_target_ptid);
> +       printf_filtered ("   gdb says parent = %ld.\n", (long) parent_pid);
> +     }
> + 
> +   fp = find_fork_pid (retpid);
> +   if (!fp)
> +     error ("Failed to find new fork");
> +   fork_save_infrun_state (fp, 1);
> + 
> +   if (info_verbose && from_tty)
> +     {
> +       printf_filtered ("retpid registers:\n");
> +       errno = 0;
> +       for (i = 0; errno == 0; i += 4)
> + 	printf_filtered ("0x%08lx\n", 
> + 			 ptrace (PTRACE_PEEKUSER, retpid, i, 0));
> +       errno = 0;
> +     }
> + }
> + 
> + #include "string.h"

That definitely won't work.  I realize it's just a debugging aid, but
PTRACE_PEEKUSER doesn't necessarily get at all registers (on lots of
targets), and doesn't necessarily work at all (the only Linux example I
know of offhand is sparc64).

Please include "gdb_string.h" (if you really wanted string.h, it would
be <string.h>); and please put it up at the top of the file with all
the other includes.


-- 
Daniel Jacobowitz
CodeSourcery, LLC


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]