From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27132 invoked by alias); 7 Aug 2008 00:42:17 -0000 Received: (qmail 27123 invoked by uid 22791); 7 Aug 2008 00:42:16 -0000 X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (66.187.233.31) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 07 Aug 2008 00:41:41 +0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id m770edM4012284; Wed, 6 Aug 2008 20:40:39 -0400 Received: from pobox.corp.redhat.com (pobox.corp.redhat.com [10.11.255.20]) by int-mx1.corp.redhat.com (8.13.1/8.13.1) with ESMTP id m770edxG020471; Wed, 6 Aug 2008 20:40:39 -0400 Received: from mesquite.lan (vpn-12-12.rdu.redhat.com [10.11.12.12]) by pobox.corp.redhat.com (8.13.1/8.13.1) with ESMTP id m770ecnB026060; Wed, 6 Aug 2008 20:40:38 -0400 Date: Thu, 07 Aug 2008 00:42:00 -0000 From: Kevin Buettner To: gdb-patches@sourceware.org Cc: luisgpm@linux.vnet.ibm.com Subject: Re: [RFC] PPC: Skip call to __eabi in main() Message-ID: <20080806174037.7cfe6445@mesquite.lan> In-Reply-To: <1217616315.29334.44.camel@gargoyle> References: <20080721155343.404977d3@mesquite.lan> <1216701867.31797.26.camel@localhost.localdomain> <20080722181252.03d9ce94@mesquite.lan> <20080728172221.3dc3764e@mesquite.lan> <1217616315.29334.44.camel@gargoyle> X-Mailer: Claws Mail 3.5.0 (GTK+ 2.12.11; i386-redhat-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-IsSubscribed: yes 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: 2008-08/txt/msg00144.txt.bz2 Hi Luis, Thanks for looking over my patch. See below for my comments... On Fri, 01 Aug 2008 15:45:14 -0300 Luis Machado wrote: > On Mon, 2008-07-28 at 17:22 -0700, Kevin Buettner wrote: > > +#define BL_MASK 0xfc000001 > > We have a very similar mask used for displaced stepping called > BRANCH_MASK (0xfc000000). It doesn't care about the LK bit though, its > purpose is just to check for a generic branch instruction. > > Maybe we should rename BL_MASK to something else incorporating the > notion that we expect a LK bit? Or maybe doing the check for the LK bit > manually in the code and using BL_MASK as is. > > > +#define BL_INSTRUCTION 0x48000001 > > Similarly, we also have B_INSN (0x48000000), lacking the LK bit. > > > +#define BL_DISPLACEMENT_MASK 0x03fffffc > > Should we make the naming more generic (B_LI_MASK maybe?) as the > displacement field is common to a variety of I-form branch instructions? > Just a suggestion, doesn't need to change if you don't feel it clarifies > things. Well, actually... I'd prefer macros like IS_BL_INSTRUCTION and BL_DISPLACEMENT that would work something like this: op = extract_unsigned_integer (buf, 4); if (IS_BL_INSTRUCTION (op)) { CORE_ADDR displ = BL_DISPLACEMENT (op); Contrast this with the code that I posted in my patch: op = extract_unsigned_integer (buf, 4); if ((op & BL_MASK) == BL_INSTRUCTION) { CORE_ADDR displ = op & BL_DISPLACEMENT_MASK; I like the first form better because it removes the explicit mask and comparison. (To be clear, these operations would still exist, but would be buried in the macro definitions.) In my opinion, the first form is easier to read and is less error prone. If we were to use this approach, we'd presumably have other macros such as IS_B_INSTRUCTION and B_DISPLACEMENT too. We could write a version of B_DISPLACMENT which would work for BL instructions, but I would prefer to have separate macros for each case because it's easier to verify correctness. Consider the following: if (IS_BL_INSTRUCTION (op)) { CORE_ADDR displ = B_DISPLACEMENT (op); Many times, we create new code by copying and pasting existing code. If I, as a patch reviewer, were to see this in a patch, I might wonder if B_DISPLACEMENT also works for BL instructions and would ultimately have to refer to the macro and consult an instruction manual to verify that the code is correct. Using a separate BL-specific macro eliminates any concern that either a typo was made or that an existing hunk of code was copied and not entirely converted. With regard to my patch, I'd prefer to commit it in its present form and then address improvements to PowerPC instruction decoding at a later time. I considered using my preferred approach when I adjusted my patch, but decided against doing so because a different approach (that of using explicit masks and comparisons) was already in use. FWIW, this isn't the only approach that I find compelling. I recently worked on a port which utilizes the instruction decoder in opcodes/. This decoder is also used for the disassembler and simulator. It completely decodes the instruction, returning a symbolic (via enums) opcodes, and completely decoded instruction offsets, registers, etc. Kevin