From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17762 invoked by alias); 26 Apr 2011 17:09:48 -0000 Received: (qmail 17738 invoked by uid 22791); 26 Apr 2011 17:09:47 -0000 X-SWARE-Spam-Status: No, hits=-1.3 required=5.0 tests=AWL,BAYES_00,MSGID_FROM_MTA_HEADER,SPF_SOFTFAIL,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mtagate2.uk.ibm.com (HELO mtagate2.uk.ibm.com) (194.196.100.162) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 26 Apr 2011 17:09:29 +0000 Received: from d06nrmr1507.portsmouth.uk.ibm.com (d06nrmr1507.portsmouth.uk.ibm.com [9.149.38.233]) by mtagate2.uk.ibm.com (8.13.1/8.13.1) with ESMTP id p3QH9Ltw022053 for ; Tue, 26 Apr 2011 17:09:21 GMT Received: from d06av02.portsmouth.uk.ibm.com (d06av02.portsmouth.uk.ibm.com [9.149.37.228]) by d06nrmr1507.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p3QHAOjt1470656 for ; Tue, 26 Apr 2011 18:10:24 +0100 Received: from d06av02.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p3QH9KLX026821 for ; Tue, 26 Apr 2011 11:09:21 -0600 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with SMTP id p3QH9JbU026808; Tue, 26 Apr 2011 11:09:19 -0600 Message-Id: <201104261709.p3QH9JbU026808@d06av02.portsmouth.uk.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Tue, 26 Apr 2011 19:09:19 +0200 Subject: Re: [try 2nd 2/8] Rename copy_* functions to arm_copy_* To: yao@codesourcery.com (Yao Qi) Date: Tue, 26 Apr 2011 17:09:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org In-Reply-To: <4D9D6F85.1030008@codesourcery.com> from "Yao Qi" at Apr 07, 2011 04:02:13 PM 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: 2011-04/txt/msg00481.txt.bz2 Yao Qi wrote: > On 04/07/2011 04:51 AM, Ulrich Weigand wrote: > > I'd prefer if this were handled in a regular manner: the copy_ > > routines parse the insn text, extract all required information > > and pass it all as arguments to the install_ routine. The > > install_ routine then stores all information that is needed > > by the cleanup_ routine into the displaced_step_closure struct. > > From the consistency's point of view, I am fine with your approach. In > this new patch, the following things are changed, > 1. Merge 6/8 patch, > 2. Pass all values as parameters to install_* routines, > 3. Fix install_* routines' arguments order, > 4. Change all install_* routines to void method. > > This patches makes patch 4/8 and 5/8 obsolete. I'll re-send an updated > version once this patch is approved. Sorry for the delay; I've been out of the office for a couple of weeks. Thanks for making those changes, this is looking very good now. There's just a couple of minor issues: > + dsc->u.ldst.writeback = writeback; > + dsc->u.ldst.rn = rn; > + > dsc->tmp[0] = displaced_read_reg (regs, dsc, 0); > - rn_val = displaced_read_reg (regs, dsc, rn); > + rn_val = displaced_read_reg (regs, dsc, dsc->u.ldst.rn); > displaced_write_reg (regs, dsc, 0, rn_val, CANNOT_WRITE_PC); > > - dsc->u.ldst.writeback = bit (insn, 25); > - dsc->u.ldst.rn = rn; These changes are really unnecessary now, except for replacing "bit (insn, 25)" by "writeback" at the end. Using the local arguments "rn" instead of struct accesses like "dsc->u.ldst.rn" keeps the code more readable (and the patch smaller), so it would be somewhat preferable. Similar unnecessary changes are in a couple of other install_ routines. For some reason, just three of the copy_ routines are not renamed to arm_copy_ (even though they are of course ARM specific): copy_extra_ld_st, copy_block_xfer, copy_unpred Please rename those as well (and any others I may have missed). Finally, the patch renames two of the "decode_" routines to "arm_decode_", but not the others. Shouldn't they all be renamed? (Of course this is really independent of the rest of the patch, so maybe all those renamed ought to be done in a separate patch.) Otherwise, this looks OK now. Thanks, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com