From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24903 invoked by alias); 14 May 2010 12:20:25 -0000 Received: (qmail 24887 invoked by uid 22791); 14 May 2010 12:20:24 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,TW_BJ,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; Fri, 14 May 2010 12:20:20 +0000 Received: (qmail 21491 invoked from network); 14 May 2010 12:20:18 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 14 May 2010 12:20:18 -0000 From: Pedro Alves To: Hui Zhu Subject: Re: [RFA] Checkpoint: wait the defunct process when delete it Date: Fri, 14 May 2010 14:43:00 -0000 User-Agent: KMail/1.12.2 (Linux/2.6.31-21-generic; KDE/4.3.2; x86_64; ; ) Cc: Michael Snyder , "gdb-patches@sourceware.org" References: <201005131130.57597.pedro@codesourcery.com> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201005141320.09678.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: 2010-05/txt/msg00299.txt.bz2 On Friday 14 May 2010 07:39:17, Hui Zhu wrote: > > BTW, you patch is not error/exception safe. You didn't consider > > the case of an `error' being thrown between switching the > > fork and back. > > Thanks for alarm me about it. I have add a > inferior_call_waitpid_cleanup to clean up And move > inferior_call_waitpid to the end of function > delete_checkpoint_command. Then it will not affect inferior_ptid and > delete_fork (ptid); You're now accessing the FI pointer after having just deleted it. The delete_fork(ptid) line deletes FI. > + fi = find_fork_ptid (ptid); > + gdb_assert (fi); > + > if (from_tty) > printf_filtered (_("Killed %s\n"), target_pid_to_str (ptid)); > > delete_fork (ptid); > + > + /* If fi->parent_ptid is not a part of lwp but it's a part of checkpoint > + list, waitpid the ptid. > + If fi->parent_ptid is a part of lwp and it is stoped, waitpid the > + ptid. */ > + if ((!find_thread_ptid (fi->parent_ptid) && find_fork_ptid (fi->parent_ptid)) > + || (find_thread_ptid (fi->parent_ptid) && is_stopped (fi->parent_ptid))) > + { > + if (inferior_call_waitpid (fi->parent_ptid, PIDGET (ptid))) > + warning (_("Unable to wait pid %s"), target_pid_to_str (ptid)); > + } > } > +static int > +inferior_call_waitpid (ptid_t pptid, int pid) > +{ > + struct objfile *waitpid_objf; > + struct value *waitpid_fn = NULL; > + struct value *argv[4]; > + struct gdbarch *gdbarch = get_current_arch (); > + struct fork_info *oldfp = NULL, *newfp = NULL; > + struct cleanup *old_cleanup = NULL; if (...) { > + old_cleanup = make_cleanup (inferior_call_waitpid_cleanup, oldfp); > + } > + > + if (old_cleanup) > + do_cleanups (old_cleanup); Please do not use this check-for-NULL-cleanup pattern anywhere. You should install a null_cleanup instead, and unconditionally run the cleanups. You should _never_ rely on old_cleanup being NULL to mean you haven't installed a cleanup above. The reason is that the old_cleanup = make_cleanup (inferior_call_waitpid_cleanup, oldfp); line may well return NULL, because NULL was the head of the cleanup chain when the first cleanup is registered, and that's what make_cleanup returns, the previous head of the chain. To convince yourself, put a breakpoint on make_my_cleanup and run gdb. Step out and check what is the old_chain value that ends up stored in the caller. > + return ret; > +} I see you dropped extracting the return value of the waitpid call, in addition to the dead code. Was that on purpose? It's fine either way, just pointing it out in case it slipped. -- Pedro Alves