From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 898 invoked by alias); 23 Dec 2005 14:29:46 -0000 Received: (qmail 890 invoked by uid 22791); 23 Dec 2005 14:29:45 -0000 X-Spam-Check-By: sourceware.org Received: from nevyn.them.org (HELO nevyn.them.org) (66.93.172.17) by sourceware.org (qpsmtpd/0.31.1) with ESMTP; Fri, 23 Dec 2005 14:29:43 +0000 Received: from drow by nevyn.them.org with local (Exim 4.54) id 1EpnvQ-0006jp-V3; Fri, 23 Dec 2005 09:29:41 -0500 Date: Fri, 23 Dec 2005 22:42:00 -0000 From: Daniel Jacobowitz To: Michael Snyder Cc: gdb-patches@sources.redhat.com, msnyder@redhat.com Subject: Re: [RFA] Linux Checkpoint/Restart, take 2 Message-ID: <20051223142940.GA24997@nevyn.them.org> Mail-Followup-To: Michael Snyder , gdb-patches@sources.redhat.com, msnyder@redhat.com References: <43A777EE.50404@cisco.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <43A777EE.50404@cisco.com> User-Agent: Mutt/1.5.8i X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2005-12/txt/msg00275.txt.bz2 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 Return the program state to a > + previously saved state. > + > + info checkpoints List currently saved checkpoints. > + > + delete-checkpoint 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 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 Delete a fork from the list of forks > + that are available to be debugged (and > + kill the forked process). > + > + detach-fork 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 ); and please put it up at the top of the file with all the other includes. -- Daniel Jacobowitz CodeSourcery, LLC