From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21174 invoked by alias); 20 Dec 2009 13:48:12 -0000 Received: (qmail 21158 invoked by uid 22791); 20 Dec 2009 13:48:10 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 20 Dec 2009 13:48:05 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 4D0092BAB65; Sun, 20 Dec 2009 08:48:03 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id qHZwWRxd7MGL; Sun, 20 Dec 2009 08:48:03 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id A6A572BAB63; Sun, 20 Dec 2009 08:48:02 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id B14E9F5892; Sun, 20 Dec 2009 14:30:09 +0100 (CET) Date: Sun, 20 Dec 2009 13:48:00 -0000 From: Joel Brobecker To: Michael Snyder Cc: Hui Zhu , gdb-patches ml , shuchang zhou , paawan oza Subject: Re: [RFC] Add support of software single step to process record Message-ID: <20091220133009.GI2788@adacore.com> References: <4B2BD97E.1020106@vmware.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4B2BD97E.1020106@vmware.com> User-Agent: Mutt/1.5.20 (2009-06-14) 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: 2009-12/txt/msg00281.txt.bz2 Hui: > >2009-12-18 Hui Zhu > > > > * breakpoint.c (inserted_single_step_breakpoint_p): New > > function. > > * breakpoint.h (inserted_single_step_breakpoint_p): Extern. > > * record.c (record_resume): Add code for software single step. > > (record_wait): Ditto. I understand Michael's answer as approval. I do not see any problem with it, but my knowledge of the target stack in the resume/wait area is pretty sketchy. Just a stylistic comment on the patch: > >+/* Check if the breakpoints used for software single stepping > >inserted or not. */ Formatting and missing "are". /* Check if the breakpoints used for software single stepping are inserted or not. */ > >+int > >+inserted_single_step_breakpoint_p (void) Can you rename this function to: single_step_breakpoints_inserted The "inserted" already conveys the idea of a condition/predicate, so the _p is superfluous in this case. I'm also a little worried about the code adding calls to functions that in essence return a global variable. For instance, you are introducing calls to get_current_frame or current_gdbarch(). Ulrich is one of our specialists in this area, whereas I'm not sure, but I am wondering if we are introducing any latent issue by using these routines... -- Joel