From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 916 invoked by alias); 28 Mar 2014 17:22:59 -0000 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 Received: (qmail 904 invoked by uid 89); 28 Mar 2014 17:22:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 28 Mar 2014 17:22:57 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s2SHMl9O010994 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 28 Mar 2014 13:22:47 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s2SHMjLv017470; Fri, 28 Mar 2014 13:22:45 -0400 Message-ID: <5335AFE4.2000704@redhat.com> Date: Fri, 28 Mar 2014 17:22:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Anton Blanchard CC: Joel Brobecker , gdb-patches@sourceware.org, emachado@linux.vnet.ibm.com, luis_gustavo@mentor.com, ulrich.weigand@de.ibm.com Subject: Re: [PATCH 2/4] Support up to 3 conditional branches in an atomic sequence References: <1395978111-30706-1-git-send-email-anton@samba.org> <1395978111-30706-2-git-send-email-anton@samba.org> <20140328131230.GE4030@adacore.com> <5335AD94.4030701@redhat.com> In-Reply-To: <5335AD94.4030701@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-03/txt/msg00664.txt.bz2 On 03/28/2014 05:12 PM, Pedro Alves wrote: >>> --- a/gdb/rs6000-tdep.c >>> +++ b/gdb/rs6000-tdep.c >>> @@ -1088,7 +1088,7 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame) >>> struct address_space *aspace = get_frame_address_space (frame); >>> enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); >>> CORE_ADDR pc = get_frame_pc (frame); >>> - CORE_ADDR breaks[2] = {-1, -1}; >>> + CORE_ADDR breaks[MAX_SINGLE_STEP_BREAKPOINTS]; > > If we ever bump MAX_SINGLE_STEP_BREAKPOINTS further, > you'd still only want 4 here. > > I think it'd be better if this was: > > /* 3 conditional branches + 1 terminating branch. */ > CORE_ADDR breaks[4]; > > Followed by: > > gdb_static_assert (MAX_SINGLE_STEP_BREAKPOINTS >= ARRAY_SIZE (breaks)); > > This way clearly documents that we need to support 4 sss breakpoints. > As it is, nothing in your patch leaves any indication in the source to > that effect, so the poor soul trying to revamp software single-step > breakpoints could miss this. Sorry, I wrote this before realizing that there could even be more condition branches in the region and writing the "is there a hard limit", and then pushed send too soon. So assuming there's no limit and we're just trying to be practical in what we support, there's really no harm in leaving this as: CORE_ADDR breaks[MAX_SINGLE_STEP_BREAKPOINTS]; but I still think a comment (+ the assert would be nice) is missing. Something like: /* The ppc64 Linux kernel has regions with 3 conditional branches. (plus 1 for the terminating branch). */ gdb_static_assert (MAX_SINGLE_STEP_BREAKPOINTS >= 4); ? BTW, shouldn't GDB warn or even error out if too many conditional branches are found? I think that'd be better than silently getting stuck forever. -- Pedro Alves