From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19839 invoked by alias); 13 Jun 2012 16:02:29 -0000 Received: (qmail 19785 invoked by uid 22791); 13 Jun 2012 16:02:26 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_NO 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; Wed, 13 Jun 2012 16:02:13 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 7A3FC1C724E; Wed, 13 Jun 2012 12:02:12 -0400 (EDT) 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 eDtpAjIsnVws; Wed, 13 Jun 2012 12:02:12 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 29B141C724D; Wed, 13 Jun 2012 12:02:12 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id B22C4145616; Wed, 13 Jun 2012 09:02:08 -0700 (PDT) Date: Wed, 13 Jun 2012 16:02:00 -0000 From: Joel Brobecker To: Anton Blanchard Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 2/3] Support up to 3 conditional branches in an atomic sequence Message-ID: <20120613160208.GH18729@adacore.com> References: <20120606135557.7da37cbe@kryten> <20120606135655.57bd5b54@kryten> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120606135655.57bd5b54@kryten> 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: 2012-06/txt/msg00421.txt.bz2 Anton, Thanks for the patch. I am not a real specialist of the software single-step implementation, even though I worked on it a really long time ago. But your changes mostly make sense to me (and seem to be relatively mechanical in nature). > 2012-06-05 Anton Blanchard > > * gdb/breakpoint.h: Define NR_SINGLE_STEP_BREAKPOINTS > * rs6000-tdep.c (ppc_deal_with_atomic_sequence): Allow for more > than two breakpoints. > * gdb/breakpoint.c (insert_single_step_breakpoint): Likewise > (insert_single_step_breakpoint): Likewise > (single_step_breakpoints_inserted): Likewise > (cancel_single_step_breakpoints): Likewise > (detach_single_step_breakpoints): Likewise > (single_step_breakpoint_inserted_here_p): Likewise Mostly OK. I would like you to change the name of the macro to MAX_SINGLE_STEP_BREAKPOINTS, though. Can you, please? Other comments and questions inline. > - int bc_insn_count = 0; /* Conditional branch instruction count. */ Thanks for deleting that variable that seems redundant with last_breakpoint... > + if (last_breakpoint >= (NR_SINGLE_STEP_BREAKPOINTS-1)) > + return 0; /* too many conditional branches found, fallback Can you remove the extra parens which are useless in this case? And binary operators should have a space before and after. Thus: if (last_breakpoint > MAX_SINGLE_STEP_BREAKPOINTS - 1) > Index: gdb/gdb/breakpoint.h [...] > -/* Manage a software single step breakpoint (or two). Insert may be > - called twice before remove is called. */ > +/* Manage software single step breakpoints. */ > +#define NR_SINGLE_STEP_BREAKPOINTS 4 > + Just curious: Why did you remove the second sentence from the comment? Is it no longer true? Maybe it is the "twice" that should be changed into "multiple times"? Thanks, -- Joel