From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20998 invoked by alias); 28 Mar 2014 17:13:09 -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 20985 invoked by uid 89); 28 Mar 2014 17:13:07 -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:13:06 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s2SHCtLk023427 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 28 Mar 2014 13:12:55 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s2SHCqi5027962; Fri, 28 Mar 2014 13:12:53 -0400 Message-ID: <5335AD94.4030701@redhat.com> Date: Fri, 28 Mar 2014 17:13: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: Joel Brobecker CC: Anton Blanchard , gdb-patches@sourceware.org, emachado@linux.vnet.ibm.com, luis_gustavo@mentor.com, ulrich.weigand@de.ibm.com, Pedro Alves 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> In-Reply-To: <20140328131230.GE4030@adacore.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-03/txt/msg00663.txt.bz2 On 03/28/2014 01:12 PM, Joel Brobecker wrote: > On Fri, Mar 28, 2014 at 02:41:49PM +1100, Anton Blanchard wrote: >> gdb currently supports 1 conditional branch inside a ppc larx/stcx >> critical region. Unfortunately there is existing code that contains more >> than 1, for example in the ppc64 Linux kernel: >> >> c00000000003ac18 <.__hash_page_4K>: >> ... >> c00000000003ac4c: ldarx r31,0,r6 >> c00000000003ac50: andc. r0,r4,r31 >> c00000000003ac54: bne- c00000000003aee8 >> c00000000003ac58: andi. r0,r31,2048 >> c00000000003ac5c: bne- c00000000003ae0c >> c00000000003ac60: rlwinm r30,r4,30,24,24 >> c00000000003ac64: or r30,r30,r31 >> c00000000003ac68: ori r30,r30,2304 >> c00000000003ac6c: oris r30,r30,4096 >> c00000000003ac70: stdcx. r30,0,r6 >> >> If we try to single step through this we get stuck forever because >> the reservation is never set when we step over the stdcx. >> >> The following patch bumps the number to 3 conditional branches + 1 >> terminating branch. With this patch applied I can single step through >> the offending function in the ppc64 Linux kernel. Is there a hard limit? Like, is there a limit on the number of instructions that can appear inside a ppc larx/stcx region? >> >> gdb/ >> 2014-03-28 Anton Blanchard >> >> * breakpoint.h (MAX_SINGLE_STEP_BREAKPOINTS): New define. >> * rs6000-tdep.c (ppc_deal_with_atomic_sequence): Allow for more >> than two breakpoints. >> * 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. > > Overall, this looks like a nice generalization, but Pedro has been > more active in the breakpoint area than I have, so it would be nice > to have his feedback on this patch as well. > > IIUC, it looks like MAX_SINGLE_STEP_BREAKPOINTS is actually not > the max, but MAX - 1. I was a little confused by that. Why not > change MAX to 3, and then adjust the array definition and code > accordingly? I think things will be slightly simpler to understand. IMO that would be more confusing. I read MAX_SINGLE_STEP_BREAKPOINTS as meaning the "maximum number of of single-step breakpoints we can create simultaneously". I think you're reading it as "the highest index possible into the single-step breakpoints array" ? Maybe it'd be clearer to just rename the macro, to, say NUM_SINGLE_STEP_BREAKPOINTS? >> + for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++) >> + if (single_step_breakpoints[i] == NULL) >> + break; I notice something odd with the formatting. E.g., above, vs: >> + for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++) >> + if (single_step_breakpoints[i] != NULL) >> + return 1; Probably tab vs spaces -- please make use to use tabs for 8 spaces. >> --- 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. >> CORE_ADDR loc = pc; >> CORE_ADDR closing_insn; /* Instruction that closes the atomic sequence. */ >> int insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order); >> @@ -1097,7 +1097,6 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame) >> int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed). */ >> const int atomic_sequence_length = 16; /* Instruction sequence length. */ >> int opcode; /* Branch instruction's OPcode. */ >> - int bc_insn_count = 0; /* Conditional branch instruction count. */ >> >> /* Assume all atomic sequences start with a lwarx/ldarx instruction. */ >> if ((insn & LWARX_MASK) != LWARX_INSTRUCTION >> @@ -1111,24 +1110,20 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame) >> loc += PPC_INSN_SIZE; >> insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order); >> >> - /* Assume that there is at most one conditional branch in the atomic >> - sequence. If a conditional branch is found, put a breakpoint in >> - its destination address. */ >> if ((insn & BRANCH_MASK) == BC_INSN) >> { >> int immediate = ((insn & 0xfffc) ^ 0x8000) - 0x8000; >> int absolute = insn & 2; >> >> - if (bc_insn_count >= 1) >> - return 0; /* More than one conditional branch found, fallback >> + if (last_breakpoint >= MAX_SINGLE_STEP_BREAKPOINTS - 1) >> + return 0; /* too many conditional branches found, fallback No need for '>=' IFAICS. Here I'd suggest: if (last_breakpoint == ARRAY_SIZE (breaks) - 1) { /* Too many conditional branches found, fallback to the standard single-step code. */ return 0; } Note "Too" should be capitalized. also, our style nowadays says to wrap the comment and statement in a {}s if it's multiline, even though the old code wasn't doing that. With those changes this looks good to me. -- Pedro Alves >> >> if (absolute) >> - breaks[1] = immediate; >> + breaks[last_breakpoint] = immediate; >> else >> - breaks[1] = loc + immediate; >> + breaks[last_breakpoint] = loc + immediate; >> >> - bc_insn_count++; >> last_breakpoint++; >> } >> >> @@ -1147,18 +1142,29 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame) >> insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order); >> >> /* Insert a breakpoint right after the end of the atomic sequence. */ >> - breaks[0] = loc; >> + breaks[last_breakpoint] = loc; >> >> - /* Check for duplicated breakpoints. Check also for a breakpoint >> - placed (branch instruction's destination) anywhere in sequence. */ >> - if (last_breakpoint >> - && (breaks[1] == breaks[0] >> - || (breaks[1] >= pc && breaks[1] <= closing_insn))) >> - last_breakpoint = 0; >> - >> - /* Effectively inserts the breakpoints. */ >> for (index = 0; index <= last_breakpoint; index++) >> - insert_single_step_breakpoint (gdbarch, aspace, breaks[index]); >> + { >> + int index2; >> + int insert_bp = 1; >> + >> + /* Check for a breakpoint placed (branch instruction's destination) >> + anywhere in sequence. */ >> + if (breaks[index] >= pc && breaks[index] <= closing_insn) >> + continue; >> + >> + /* Check for duplicated breakpoints. */ >> + for (index2 = 0; index2 < index; index2++) >> + { >> + if (breaks[index] == breaks[index2]) >> + insert_bp = 0; >> + } >> + >> + /* insert the breakpoint. */ >> + if (insert_bp) >> + insert_single_step_breakpoint (gdbarch, aspace, breaks[index]); >> + } >> >> return 1; >> } >> -- >> 1.8.3.2 >