From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19048 invoked by alias); 22 Oct 2012 22:41:17 -0000 Received: (qmail 19040 invoked by uid 22791); 22 Oct 2012 22:41:17 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_NO X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 22 Oct 2012 22:41:09 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 12BD91C7D74; Mon, 22 Oct 2012 18:41:09 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 9v4Ih1n9bBqu; Mon, 22 Oct 2012 18:41:09 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 92D391C7D67; Mon, 22 Oct 2012 18:41:08 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id C7D50C7F78; Mon, 22 Oct 2012 18:41:07 -0400 (EDT) Date: Mon, 22 Oct 2012 22:41:00 -0000 From: Joel Brobecker To: Kaushik Phatak Cc: Yao Qi , "gdb-patches@sourceware.org" Subject: Re: [RFA 3/5] New port: CR16: gdb port Message-ID: <20121022224107.GB3713@adacore.com> References: <507279C7.8080401@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) 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-10/txt/msg00392.txt.bz2 > 2012-10-09 Kaushik Phatak > > gdb/Changelog > * configure.tgt: Handle cr16*-*-*linux and cr16*-*-*. > * cr16-linux-tdep.c: New file. > * cr16-tdep.c: New file. > * cr16-tdep.h: New file. This looks pretty good. It's nice to see that you're using prologue-value... My comments below: > +static const char * > +cr16_linux_register_name (struct gdbarch *gdbarch, int regnr) > +{ > + static const char *const reg_names[] = { Formatting: Could you move the '{' to the next line, please? > + /* The opcode of excp bpt is 0x00C8, however for uclinux we will > + use the excp flg (0x00C7) to insert a breakpoint. The excp bpt > + requires external hardware support for breakpoints to work on > + CR16 target. Software based breakpoints are implemented in the > + kernel using excp flg and tested on the SC14452 target. Use > + 0x00C7 with gdbserver/kernel and 0x00C8 for sim/ELF. We Trailing space at the end of this line. Can you do a pass with your editor, and make sure that they are all removed? > --- ./gdb_src.orig/gdb/cr16-tdep.c 1970-01-01 05:30:00.000000000 +0530 > +++ ./gdb_src/gdb/cr16-tdep.c 2012-10-09 18:47:25.000000000 +0530 [...] > +#include "linux-tdep.h" Intuitively, I don't think you would need to include this file from the non-linux tdep, no? > +/* Data type to store instruction opcodes. */ > +typedef unsigned short wordU; > + > +/* Globals to store opcode related information. */ > +wordU words[3]; > +ULONGLONG allWords; > +ins currInsn; These types of globals are a big no-no, and I don't really understand how they are being used. Some of them seem to never be set, others to be set but never read... Can you adjust your code to avoid those globals? > +static void > +cr16_analyze_prologue (CORE_ADDR start_pc, > + CORE_ADDR limit_pc, struct cr16_prologue *result) > +{ > + CORE_ADDR pc, next_pc; > + gdb_byte buf[6]; Can you move the declaration of this buffer to the scope where it is used, please? It's clearer that way. > + char insn_byte1, insn_byte2; I think you probably want to use gdb_byte, here. > + /* If PUSH, then save RA and other regs. */ > + if (insn_byte1 == 0x01) > + { > + int r1, r2; > + int r; > + insn_byte2 = words[0]; Empty line between variable declarations and the rest of the code. There are other locations where I saw the same issue. Can you do a pass over those, please? > +static struct cr16_prologue * > +cr16_analyze_frame_prologue (struct frame_info *this_frame, > + void **this_prologue_cache) > +{ > + if (!*this_prologue_cache) This is a bit of a nit, and you are welcome to keep the code as is, but the typical usage in this case is to do: if (*this_prologue_cache) return *this_prologue_cache; This allows to have the rest of the code less indented, which is usually more easily readable. > +static CORE_ADDR > +cr16_unwind_pc (struct gdbarch *gdbarch, struct frame_info *this_frame) > +{ > + ULONGEST pc; > + > + pc = frame_unwind_register_unsigned (this_frame, CR16_PC_REGNUM); > + return pc; > +} Just curious: Why not just: return frame_unwind_register_unsigned (this_frame, CR16_PC_REGNUM); I have no doubt that the compiler is going to remove the local variable, and so we'll end up with the same code. But then you have your local variable's type being different from the return value type. > + > +/* Implement the "unwind_sp" gdbarch method. */ > + > +static CORE_ADDR > +cr16_unwind_sp (struct gdbarch *gdbarch, struct frame_info *this_frame) Same here. > + u = > + extract_unsigned_integer (arg_bits, arg_size, byte_order); Nit: I think it's fine to format the entire statement into a single line. It looks like it just fits 80 chars. Otherwise, i'd format it like u = extract_unsigned_integer (arg_bits, arg_size, byte_order); > + /* If we don't pass the option -mint32 > + FIXME: add if else case depending on the option passed, > + sp that we can have int size as 16 or 32 bits both. */ Is there any way of handling this fixme easily? > +/* Implement the "register_name" gdbarch method. */ > + > +static const char * > +cr16_register_name (struct gdbarch *gdbarch, int regnr) > +{ > + static const char *const reg_names[] = { Formatting - same as above. > + default: > + printf > + ("\nRegister Type not supported\nFunction : cr16_register_type\n"); > + return 0; > + break; You cannot do that: We assume that that this function never NULL. Depending on the reason for not "supporting" certain registers, you can add a gdb_assert_not_reached if you know you've covered all registers, or else return any type, like builtin_int32. -- Joel