From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30233 invoked by alias); 12 Apr 2011 12:55:20 -0000 Received: (qmail 30225 invoked by uid 22791); 12 Apr 2011 12:55:19 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 12 Apr 2011 12:55:11 +0000 Received: (qmail 805 invoked from network); 12 Apr 2011 12:55:10 -0000 Received: from unknown (HELO scottsdale.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 12 Apr 2011 12:55:10 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [PATCH] GDB checkpoint can't/shouldn't be possible with multiple threads on Linux Date: Tue, 12 Apr 2011 12:55:00 -0000 User-Agent: KMail/1.13.5 (Linux/2.6.35-28-generic; KDE/4.6.2; x86_64; ; ) Cc: Kevin Pouget References: <201104121159.06026.pedro@codesourcery.com> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201104121355.11278.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-04/txt/msg00166.txt.bz2 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 > > 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