From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20662 invoked by alias); 30 Jan 2012 22:44:05 -0000 Received: (qmail 20651 invoked by uid 22791); 30 Jan 2012 22:44:04 -0000 X-SWARE-Spam-Status: No, hits=-5.4 required=5.0 tests=AWL,BAYES_50,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_XF,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 30 Jan 2012 22:43:50 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q0UMhoro004794 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 30 Jan 2012 17:43:50 -0500 Received: from mesquite.lan (ovpn-113-83.phx2.redhat.com [10.3.113.83]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q0UMhnZQ024373 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO) for ; Mon, 30 Jan 2012 17:43:49 -0500 Date: Mon, 30 Jan 2012 23:05:00 -0000 From: Kevin Buettner To: gdb-patches@sourceware.org Subject: Re: [RFC] Add support for the Renesas rl78 architecture Message-ID: <20120130154348.31f04780@mesquite.lan> In-Reply-To: <4F2515EA.9060505@codesourcery.com> References: <20120125165800.5351c291@mesquite.lan> <4F2515EA.9060505@codesourcery.com> 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: 2012-01/txt/msg01003.txt.bz2 Hi Yao, Thanks for looking at my patch. With regard to the blank lines after declarations, I've made those changes to rl78-tdep.c. I will post an update in a few minutes in case you'd like to spot check this. I'll address your other concerns below... On Sun, 29 Jan 2012 17:48:26 +0800 Yao Qi wrote: > On 01/26/2012 07:58 AM, Kevin Buettner wrote: > > > +/* Strip bits to form an instruction address. (When fetching a > > + 32-bit address from the stack, the high eight bits are garbage. > > + This function strips off those unused bits.) */ > > +static CORE_ADDR > > +rl78_make_instruction_address (CORE_ADDR addr) > > +{ > > + return addr & 0xffffff; > > +} > > + > > +/* Set / clear bits necessary to make a data address. */ > > +static CORE_ADDR > > +rl78_make_data_address (CORE_ADDR addr) > > +{ > > + return (addr & 0xffff) | 0xf0000; > > +} > > + > > Why can't we use macro instead of function here? We could use a macro here. When I've attempted to use macros in the past, I was told "Macros are bad, M'kay." The GDB internals doc even says this. (That's an exact quote.) See section 16.1.4: http://sourceware.org/gdb/current/onlinedocs/gdbint/Coding-Standards.html I think today's compiler technology makes macro use for efficiency reasons much less compelling than it once was. > > +/* Implement the "breakpoint_from_pc" gdbarch method. */ > > +const gdb_byte * > > +rl78_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr, int *lenptr) > > +{ > > + static gdb_byte breakpoint[] = { 0xff }; > > + > > + /* The above are the bytes required for the BRK instruction. However, > > + instructions can be as short as one byte. The simulator looks for > > + memory writes to program areas using this pattern, however, and > > + implements breakpoints via a different mechanism. */ > > The code is clear to me, but the comment here is confusing. > > > + *lenptr = sizeof breakpoint; > > + return breakpoint; > > +} Thank you for catching this. I used to have a two byte breakpoint sequence there. We later learned of a one byte sequence that could be used and I forgot to update the comment. I've revised that function to appear as shown below... /* Implement the "breakpoint_from_pc" gdbarch method. */ const gdb_byte * rl78_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr, int *lenptr) { /* The documented BRK instruction is actually a two byte sequence, {0x61, 0xcc}, but instructions may be as short as one byte. Correspondence with Renesas revealed that the one byte sequence 0xff is used when a one byte breakpoint instruction is required. */ static gdb_byte breakpoint[] = { 0xff }; *lenptr = sizeof breakpoint; return breakpoint; } > > +/* Implement the "pointer_to_address" gdbarch method. */ > > +static CORE_ADDR > > +rl78_pointer_to_address (struct gdbarch *gdbarch, > > + struct type *type, const gdb_byte *buf) > > +{ > > + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > > + CORE_ADDR addr > > + = extract_unsigned_integer (buf, TYPE_LENGTH (type), byte_order); > > + > > + /* Is it a code address? */ > > + if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC > > + || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD > > + || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type)) > > + || TYPE_LENGTH (type) == 4 > > If a pointer points to an integer (it is a data address, and size is > 4-byte), we will get the instruction address, which is not correct, IMO. I agree with what you're saying, but I'm checking the size of the pointer here. (Well, that's my intention anyway.) Code pointers can be 4 bytes in size. Data pointers are 2 bytes in size. > > + ) > > I think this parenthesis should be put in previous line, IMO. I agree. Done. > > +static CORE_ADDR > > +rl78_unwind_pc (struct gdbarch *arch, struct frame_info *next_frame) > > +{ > > + struct gdbarch_tdep *tdep = gdbarch_tdep (arch); > > `tdep' is not used. I've removed this declaration as well as the other unused tdep declaration that you found too. Thanks again for your detailed review. It is much appreciated. Kevin