From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28623 invoked by alias); 6 May 2007 21:20:47 -0000 Received: (qmail 28615 invoked by uid 22791); 6 May 2007 21:20:46 -0000 X-Spam-Check-By: sourceware.org Received: from mtagate6.de.ibm.com (HELO mtagate6.de.ibm.com) (195.212.29.155) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sun, 06 May 2007 21:20:44 +0000 Received: from d12nrmr1607.megacenter.de.ibm.com (d12nrmr1607.megacenter.de.ibm.com [9.149.167.49]) by mtagate6.de.ibm.com (8.13.8/8.13.8) with ESMTP id l46LKgk6178492 for ; Sun, 6 May 2007 21:20:42 GMT Received: from d12av02.megacenter.de.ibm.com (d12av02.megacenter.de.ibm.com [9.149.165.228]) by d12nrmr1607.megacenter.de.ibm.com (8.13.8/8.13.8/NCO v8.3) with ESMTP id l46LKf7n4165814 for ; Sun, 6 May 2007 23:20:41 +0200 Received: from d12av02.megacenter.de.ibm.com (loopback [127.0.0.1]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id l46LKfkn023988 for ; Sun, 6 May 2007 23:20:41 +0200 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.12.11) with SMTP id l46LKf9l023985; Sun, 6 May 2007 23:20:41 +0200 Message-Id: <200705062120.l46LKf9l023985@d12av02.megacenter.de.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Sun, 6 May 2007 23:20:41 +0200 Subject: Re: [RFC] "single step" atomic instruction sequences as a whole on PPC To: luisgpm@linux.vnet.ibm.com Date: Sun, 06 May 2007 21:20:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org In-Reply-To: <1178203875.4375.8.camel@localhost> from "Luis Machado" at May 03, 2007 11:51:15 AM X-Mailer: ELM [version 2.5 PL2] MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit 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: 2007-05/txt/msg00066.txt.bz2 Luis Machado wrote: > I've modified the patch according to your suggestions. Any additional > comments on it? Thanks for the modifications. There's still a number of coding style issues that I'll point out below. > Any suggestions for the duplicate breakpoint detection? Maybe it could > be handled in a better way. Unfortunately your attempt to handle more than a single conditional branch within the sequence doesn't work, as insert_single_step_breakpoint cannot be called more than twice (it'll raise an assertion). I think you should make the assumption that there will be at most one conditional branch, but fail gracefully it that assumption is in fact violated (e.g. by issuing a warning and returning 0 to fall back on the standard single-step code). > +static int > +deal_with_atomic_sequence(struct regcache *regcache) Space before '('. > +{ > + CORE_ADDR pc = read_pc (); > + CORE_ADDR breaks[16] = {-1, -1, -1, -1, -1, -1, -1, -1, > + -1, -1, -1, -1, -1, -1, -1, -1}; > + CORE_ADDR branch_bp; /* Breakpoint at destination of a banch instruction */ Typo: "banch" Comment should end with '.' followed by two spaces. > + int index; /* Index used for the "breaks" arrays */ > + int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed) */ > + const int opcode = BC_INSTRUCTION; /* Branch instruction's OPcode */ Likewise. > + if ((insn & LWARX_MASK) != LWARX_INSTRUCTION > + && (insn & LWARX_MASK) != LDARX_INSTRUCTION) Indentation wrong. > + /* Check for conditiconal branches in the middle of the sequence Typo "conditiconal" > + branch_bp = branch_dest(opcode, insn, pc, breaks[0]); Space before '('. > + if ((insn & STWCX_MASK) == STWCX_INSTRUCTION > + || (insn & STWCX_MASK) == STDCX_INSTRUCTION) Indentation. > + /* Assume that the atomic sequence ends with a stwcx/stdcx instruction */ Comment should end with full stop. > + if ((insn & STWCX_MASK) != STWCX_INSTRUCTION > + && (insn & STWCX_MASK) != STDCX_INSTRUCTION) Indentation. > + warning (_("\nTried to step over an atomic sequence of instructions at %s\n \ > + but could not find the end of the sequence."), This leaves a lot of spaces in the warning text. > + core_addr_to_string(pc)); Space before '('. > + /* Insert a breakpoint right after the end of the atomic sequence */ Comment should end with full stop. > + /* Effectively inserts all the breakpoints */ Likewise > + for (index = 0; index < last_breakpoint; index++) > + insert_single_step_breakpoint (breaks[index]); > + > + gdb_flush (gdb_stdout); I don't think this is necessary. > + if (deal_with_atomic_sequence(regcache)) Space before '('. > + /* Handles single stepping of atomic sequences */ Comment should end with full stop. > + set_gdbarch_software_single_step(gdbarch, deal_with_atomic_sequence); Space before '('. Bye, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com