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

[Michael Snyder <msnyder@redhat.com>]

Daniel Jacobowitz wrote:

> 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 :-)

OK, although I have only tested it on x86.  There's really
nothing architecture-dependant in it.

> 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.

No, and I fervently hope that other targets will implement something
like it too (I may work on some of them).  However, the current patch
is for native linux only, and does not *prevent* any other targets
from implementing checkpoints.  That's why I defined all the commands
in a linux-only module.

I'd like to check in what I have now, and do the target-vectorization
as needed.  Having this implementation to play with will help us define
what target vectors we need.

> 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?

No, it actually was needed, and it's a little obscure, so I'll
give you an explanation as a footnote, down below.   ;-)

> 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.

Me too.   ;-)
But that's what the man page says....

> 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.

OK.

> 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?

Ah -- there's really only three, but one of them (in a testsuite)
got cut-n-pasted numerous times.  I'll take a look, some of 'em
are probably obsolete.  It's hard to remember to take out the
FIXME once you've fixed something.   ;-)


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

OK, will do.

> 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.

Ummm...  reasonable point, let me think about it.
Thanks for the (preliminary) feedback.

> "GNU/Linux" please.

Check.

> 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.

Well, because sometimes you'll be using checkpoints, and sometimes
you'll be debugging forks.  The underlying functionality is identical,
but users aren't gonna know or care about that -- from their point
of view, they are two entirely different tasks.


> 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).

I see what you mean -- I was just basing it off of the "thread"
command by analogy.  I'm not attached to it, though, and would be
happy to hear what others think.

> 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.

OK, well... since I have a full-fledged copy of the regcache,
there *must* be a legitimate way to do it.  If not, there should
be, and maybe I'll have to add it.   ;-)


> 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.

Ah, you mean for save_detach_fork?  You're right, I meant to
come back to that.  Thanks for catching it.


> 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).

Right, I'll just take it out.  It was just for development.

> 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.

Got it, thanks.  ;-)