From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28032 invoked by alias); 7 May 2007 22:47:13 -0000 Received: (qmail 28024 invoked by uid 22791); 7 May 2007 22:47:12 -0000 X-Spam-Check-By: sourceware.org Received: from mtagate4.de.ibm.com (HELO mtagate4.de.ibm.com) (195.212.29.153) by sourceware.org (qpsmtpd/0.31) with ESMTP; Mon, 07 May 2007 22:47:10 +0000 Received: from d12nrmr1607.megacenter.de.ibm.com (d12nrmr1607.megacenter.de.ibm.com [9.149.167.49]) by mtagate4.de.ibm.com (8.13.8/8.13.8) with ESMTP id l47Ml7Z6183946 for ; Mon, 7 May 2007 22:47:07 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 l47Ml7wx2461710 for ; Tue, 8 May 2007 00:47:07 +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 l47Ml7HL004581 for ; Tue, 8 May 2007 00:47:07 +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 l47Ml7Ht004576; Tue, 8 May 2007 00:47:07 +0200 Message-Id: <200705072247.l47Ml7Ht004576@d12av02.megacenter.de.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Tue, 8 May 2007 00:47:07 +0200 Subject: Re: [RFC] "single step" atomic instruction sequences as a whole on PPC To: luisgpm@linux.vnet.ibm.com Date: Mon, 07 May 2007 22:47:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org In-Reply-To: <1178566107.4427.16.camel@localhost> from "Luis Machado" at May 07, 2007 04:28:27 PM 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/msg00099.txt.bz2 Luis Machado wrote: > > It's much better, thanks. Unfortunately there's still a number of > > issues -- thanks for you patience in dealing with those. > > Looks like i missed some. I'm getting used to the standard, however. > Thanks for reviewing. Looks good now, thanks. > > I guess that's OK with me. I'm wondering why we need the one message > > that's still in there then -- I'd say either we warn whenever we find > > a sequence we don't understand, or we never warn. > > In order to make it more transparent to the user, i removed the messages > during the process. Fine with me. Just one more thing: the ChangeLog isn't quite right. Sorry for not noticing that earlier ... > 2007-04-13 Paul Gilliam > Luis Machado > > * rs6000-tdep.c: Defines masks for POWER instructions that set > and use the reservation flag (LWARX,LDARX,STWCX,STDCX). You need to mention those flags explicitly. > * rs6000-tdep.c (deal_with_atomic_sequence): Handles single > stepping through an atomic sequence of instructions. > * rs6000-tdep.c (rs6000_software_single_step): Added a function > call to check if we are stepping through an atomic sequence of > instructions. > * rs6000-tdep.c (rs6000_gdbarch_init): Initializes a function to > check for atomic instruction sequences while single stepping. Do not describe *why* you're changing things, only *what* you're changing. For example, in this ChangeLog (deal_with_atomic_sequence): New function. is sufficient. To describe *why* you need this function, and what it does, you should add a comment to the source file -- that's where people will look for this information. Also, if you have multiple changes to the same file, you need to list the file name only once. I'd write a ChangeLog for this patch somewhat like: * rs6000-tdep.c (LWARX_MASK, LWARX_INSTRUCTION, LDARX_INSTRUCTION, STWCX_MASK, STWCX_INSTRUCTION, STDCX_INSTRUCTION, BC_MASK, BC_INSTRUCTION): Define. (deal_with_atomic_sequence): New function. (rs6000_software_single_step): Call deal_with_atomic_sequence. (rs6000_gdbarch_init): Install deal_with_atomic_sequence as gdbarch_software_single_step routine. Can you add a comment to the patch before the new function that explains what it does and why it is needed? Thanks! > + CORE_ADDR branch_bp; /* Breakpoint at branch instruction's destination. */ > + int insn = read_memory_integer (loc, PPC_INSN_SIZE); > + int insn_count; > + int index; /* Index used for the "breaks" array. */ > + int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed). */ > + const int atomic_sequence_length = 16; /* Instruction sequence length. */ > + const int opcode = BC_INSTRUCTION; /* Branch instruction's OPcode. */ > + int bc_insn_count = 0; /* Conditional branch instruction count. */ In exchange, you might want to get rid of those comments -- that a variable called "index" is used as index doesn't need to be pointed out as comment ... With the new comment and ChangeLog, the patch is OK. Bye, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com