From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7905 invoked by alias); 28 Mar 2014 13:12:35 -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 7889 invoked by uid 89); 28 Mar 2014 13:12:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 28 Mar 2014 13:12:33 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 89559116504; Fri, 28 Mar 2014 09:12:31 -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 IqmWSp1ZgZHP; Fri, 28 Mar 2014 09:12:31 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 4122C1164FF; Fri, 28 Mar 2014 09:12:31 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id AEA55E079B; Fri, 28 Mar 2014 06:12:30 -0700 (PDT) Date: Fri, 28 Mar 2014 13:12:00 -0000 From: Joel Brobecker To: Anton Blanchard Cc: 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 Message-ID: <20140328131230.GE4030@adacore.com> References: <1395978111-30706-1-git-send-email-anton@samba.org> <1395978111-30706-2-git-send-email-anton@samba.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1395978111-30706-2-git-send-email-anton@samba.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-03/txt/msg00655.txt.bz2 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. > > 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. > --- > gdb/breakpoint.c | 63 ++++++++++++++++++++++++++++--------------------------- > gdb/breakpoint.h | 6 ++++-- > gdb/rs6000-tdep.c | 46 ++++++++++++++++++++++------------------ > 3 files changed, 62 insertions(+), 53 deletions(-) > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 19af9df..b12ea94 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -15036,11 +15036,10 @@ deprecated_remove_raw_breakpoint (struct gdbarch *gdbarch, void *bp) > return ret; > } > > -/* One (or perhaps two) breakpoints used for software single > - stepping. */ > +/* Breakpoints used for software single stepping. */ > > -static void *single_step_breakpoints[2]; > -static struct gdbarch *single_step_gdbarch[2]; > +static void *single_step_breakpoints[MAX_SINGLE_STEP_BREAKPOINTS]; > +static struct gdbarch *single_step_gdbarch[MAX_SINGLE_STEP_BREAKPOINTS]; > > /* Create and insert a breakpoint for software single step. */ > > @@ -15049,19 +15048,17 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch, > struct address_space *aspace, > CORE_ADDR next_pc) > { > + int i; > void **bpt_p; > > - if (single_step_breakpoints[0] == NULL) > - { > - bpt_p = &single_step_breakpoints[0]; > - single_step_gdbarch[0] = gdbarch; > - } > - else > - { > - gdb_assert (single_step_breakpoints[1] == NULL); > - bpt_p = &single_step_breakpoints[1]; > - single_step_gdbarch[1] = gdbarch; > - } > + for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++) > + if (single_step_breakpoints[i] == NULL) > + break; > + > + gdb_assert (i < MAX_SINGLE_STEP_BREAKPOINTS); > + > + bpt_p = &single_step_breakpoints[i]; > + single_step_gdbarch[i] = gdbarch; > > /* NOTE drow/2006-04-11: A future improvement to this function would > be to only create the breakpoints once, and actually put them on > @@ -15082,8 +15079,13 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch, > int > single_step_breakpoints_inserted (void) > { > - return (single_step_breakpoints[0] != NULL > - || single_step_breakpoints[1] != NULL); > + int i; > + > + for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++) > + if (single_step_breakpoints[i] != NULL) > + return 1; > + > + return 0; > } > > /* Remove and delete any breakpoints used for software single step. */ > @@ -15091,22 +15093,21 @@ single_step_breakpoints_inserted (void) > void > remove_single_step_breakpoints (void) > { > + int i; > + > gdb_assert (single_step_breakpoints[0] != NULL); > > /* See insert_single_step_breakpoint for more about this deprecated > call. */ > - deprecated_remove_raw_breakpoint (single_step_gdbarch[0], > - single_step_breakpoints[0]); > - single_step_gdbarch[0] = NULL; > - single_step_breakpoints[0] = NULL; > > - if (single_step_breakpoints[1] != NULL) > - { > - deprecated_remove_raw_breakpoint (single_step_gdbarch[1], > - single_step_breakpoints[1]); > - single_step_gdbarch[1] = NULL; > - single_step_breakpoints[1] = NULL; > - } > + for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++) > + if (single_step_breakpoints[i] != NULL) > + { > + deprecated_remove_raw_breakpoint (single_step_gdbarch[i], > + single_step_breakpoints[i]); > + single_step_gdbarch[i] = NULL; > + single_step_breakpoints[i] = NULL; > + } > } > > /* Delete software single step breakpoints without removing them from > @@ -15119,7 +15120,7 @@ cancel_single_step_breakpoints (void) > { > int i; > > - for (i = 0; i < 2; i++) > + for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++) > if (single_step_breakpoints[i]) > { > xfree (single_step_breakpoints[i]); > @@ -15136,7 +15137,7 @@ detach_single_step_breakpoints (void) > { > int i; > > - for (i = 0; i < 2; i++) > + for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++) > if (single_step_breakpoints[i]) > target_remove_breakpoint (single_step_gdbarch[i], > single_step_breakpoints[i]); > @@ -15151,7 +15152,7 @@ single_step_breakpoint_inserted_here_p (struct address_space *aspace, > { > int i; > > - for (i = 0; i < 2; i++) > + for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++) > { > struct bp_target_info *bp_tgt = single_step_breakpoints[i]; > if (bp_tgt > diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h > index d8e88fc..65d3ecb 100644 > --- a/gdb/breakpoint.h > +++ b/gdb/breakpoint.h > @@ -1443,8 +1443,10 @@ extern void add_solib_catchpoint (char *arg, int is_load, int is_temp, > deletes all breakpoints. */ > extern void delete_command (char *arg, int from_tty); > > -/* Manage a software single step breakpoint (or two). Insert may be > - called twice before remove is called. */ > +/* Manage software single step breakpoints. Insert may be > + called multiple times before remove is called. */ > +#define MAX_SINGLE_STEP_BREAKPOINTS 4 > + > extern void insert_single_step_breakpoint (struct gdbarch *, > struct address_space *, > CORE_ADDR); > diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c > index dbe3aa2..be14e39 100644 > --- 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]; > 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 > to the standard single-step code. */ > > 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 -- Joel