Re: [PATCH] GDB checkpoint can't/shouldn't be possible with multiple threads on Linux

[Pedro Alves <pedro at codesourcery dot com>]

On Tuesday 12 April 2011 12:55:05, Kevin Pouget wrote:
> > Next time please post ChangeLog entries as plaintext,
> > separate from the patch.
> 
> 2011-04-12 Kevin Pouget <kevin.pouget@st.com>
> 
> 	PR threads/12628
> 
> 	* linux-fork.c (checkpoint_command): Disallow checkpointing of
> 	processes with multiple threads.
> 	(inf_has_multiple_thread_cb): New function.
> 
> > > diff --git a/gdb/linux-fork.c b/gdb/linux-fork.c
> > > index 7f654af..c137604 100644
> > > --- a/gdb/linux-fork.c
> > > +++ b/gdb/linux-fork.c
> > > @@ -628,6 +628,11 @@ checkpoint_command (char *args, int from_tty)
> > >    pid_t retpid;
> > >    struct cleanup *old_chain;
> > >
> > > +  /* Ensure that the inferior is not multithreaded. */
> >
> > Double space after periods.
> 
> fixed
> 
> > > +  update_thread_list () ;
> > > +  if (thread_count () > 1)
> > > +    error (_("checkpoint: can't checkpoint multiple threads.")) ;
> >
> > You have spurious spaces before `;'.
> 
> fixed
> 
> > thread_count() returns the sum total number of threads of all
> > inferiors/process.  You want the number of threads of the
> > current process only.  AFAIR, there's no function handy that
> > returns you that.  (Since you're interested in checking for multiple
> > threads, you could use iterate_over_threads with a
> > callback that returns true if it sees a second thread for a
> > given process, so you don't really need to count all
> > the threads.)
> 
> you're right, I updated the patch as suggested

Can you have a dialogue with your mailer, convincing it
to attach text files as Content-Type: text/$SOMETHING
rather than application/octet-stream + base64 ?
It's more steps to to read and harder to quote-for-review
binary attachments than should be necessary.  Thanks.

diff --git a/gdb/linux-fork.c b/gdb/linux-fork.c
index 7f654af..8a18d42 100644
--- a/gdb/linux-fork.c
+++ b/gdb/linux-fork.c
@@ -616,6 +616,20 @@ linux_fork_checkpointing_p (int pid)
   return (checkpointing_pid == pid);
 }
 
> +static int
> +inf_has_multiple_thread_cb (struct thread_info *tp, void *data)

Misses a short describing comment.  Something like

/* Callback for iterate over threads.  Used to check whether
   the current inferior is multi-threaded.  Returns true as soon
   as it sees the second thread of the current inferior.  */

> +{
> +  int *has_multiple_threads = (int *) data;
> +  
> +  gdb_assert(current_inferior() != NULL);

(Many other things would break if this wasn't true.)
Missing spaces before `('.

> +  if (current_inferior()->pid == GET_PID(tp->ptid))

Missing spaces before `('.  Please use ptid_get_pid.

  if (current_inferior ()->pid == ptid_get_pid (tp->ptid))

> +    (*has_multiple_threads)++;
> +  
> +  /* Stop the iteration if multiple threads have been detected.  */
> +  return *has_multiple_threads > 1;
> +}
> +
>  static void
>  checkpoint_command (char *args, int from_tty)
>  {
> @@ -627,7 +641,16 @@ checkpoint_command (char *args, int from_tty)
>    struct fork_info *fp;
>    pid_t retpid;
>    struct cleanup *old_chain;
> +  int has_multiple_threads = 0 ;

Space before `;' again.  Let's reserve variables named
`has_FOO' for booleans.  Otherwise, the code is harder
to read (the "if (has_multiple_threads > 1)" check below
made me go "hmm?").  You could encapsulate the magic count
in a predicate function:

static int
inf_has_multiple_threads (void)
{
  int count = 0;

  iterate_over_threads (inf_has_multiple_thread_cb, &count);
  return (count > 1);
}

> +  iterate_over_threads(inf_has_multiple_thread_cb, &has_multiple_threads);
> +  if (has_multiple_threads > 1)
> +    error (_("checkpoint: can't checkpoint multiple threads."));


> +
> +  if (!target_has_execution) error (_("The program is not being run."));

Each statement goes on its own line. 

> +  /* Ensure that the inferior is not multithreaded.  */
> +  update_thread_list () ;

Space before `;' again.

> +  iterate_over_threads(inf_has_multiple_thread_cb, &has_multiple_threads);
> +  if (has_multiple_threads > 1)
> +    error (_("checkpoint: can't checkpoint multiple threads."));
> +  

Missing spaces again.

-- 
Pedro Alves