From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26539 invoked by alias); 4 Oct 2012 14:48:25 -0000 Received: (qmail 26414 invoked by uid 22791); 4 Oct 2012 14:48:23 -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; Thu, 04 Oct 2012 14:48:16 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 0E8241C7E8F; Thu, 4 Oct 2012 10:48:16 -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 lMEEGL2NhkbP; Thu, 4 Oct 2012 10:48:15 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id D13B51C7E66; Thu, 4 Oct 2012 10:48:15 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 87908CB4C8; Thu, 4 Oct 2012 16:48:06 +0200 (CEST) Date: Thu, 04 Oct 2012 14:48:00 -0000 From: Joel Brobecker To: Kaushik Phatak Cc: "gdb-patches@sourceware.org" Subject: Re: [RFA 3/5] New port: CR16: gdb port Message-ID: <20121004144806.GO3028@adacore.com> References: 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/msg00077.txt.bz2 > 2012-10-04 Kaushik Phatak > > gdb/Changelog > * cr16-linux-tdep.c: New file. > * cr16-tdep.c: New file. > * cr16-tdep.h: New file. A good start :-). I am running out of time for today, but I noticed a few things you can work on right away: The biggie is that I think that there is a log of stuff in cr16-tdep that is actually linux-specific, and that you should move to cr16-linux-tdep.c. You'll see that it'll simplify your gdbarch_init phase a bit (the linux one will probably build on top of the bareboard one). also, everything entity needs to be properly documented. This applies to new types, new constants, global variables and functions. Therefore: > +const gdb_byte breakpoint_uclinux[] = { 0xC7, 0x00 }; Needs a comment. For global variables and constants, I think that the general practice is to put the comment on the line just before the definition: /* The breakpoint instruction to use when bla bla bla. */ const gdb_byte breakpoint_uclinux[] = { 0xC7, 0x00 }; For functions and types, however, we decided to have an empty line between the documentation/comment and the function. On formatting nits... (sorry - the GDB project is a little picky about this, but for good reasons, IMO) > +static void > +cr16_uclinux_init_abi (struct gdbarch_info info, > + struct gdbarch *gdbarch) The last line isn't aligned properly. > + /* 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 represent the breakpoint in little endian format since CR16 supports > + only little endian. > + */ The recommended line limit is 70, with a hard limit of 80 characters. So this paragraph needs to be reformatted. Also, the GNU Coding Style requires that 2 spaces be used after periods. This applies for instance to the second line. Please also move the closing "*/" to the end of the last line, with two spaces after the last period. I will not highlight the other occurences of these formatting issues. Can you make a pass over your code? > +typedef unsigned short wordU; > +extern wordU words[3]; > +extern ULONGLONG allWords; > +extern ins currInsn; This looks strange. Why extern, and where would they come from? Needs a comment. > +/* Use functions from opcodes/cr16-dis.c by making them non-static */ > +extern void make_instruction (void); > +extern int match_opcode (void); > +extern void get_words_at_PC (bfd_vma memaddr, struct disassemble_info *info); Don't these functions have declarations in opcode? This is really ugly, and potentially dangerous if the function's profile get changed at some point in the future. Please consider adding these to a header file in opcodes, and have both opcodes and GDB inclulde those, to ensure consistency. > +static const struct frame_unwind cr16_frame_unwind = { Formatting nit: The '{' needs to be on the next line. There are probably other instances of this issue. > + /* We use different breakpoint instructions for ELF and uClinux. > + See cr16-linux-tdep.c for more details. > + */ Trailing space at the end of the second line. Please also move the closing */ to the second line as well. > +extern const gdb_byte breakpoint_elf[]; > +extern const gdb_byte breakpoint_linux[]; I don't think these need to be in the .h file. I couldn't find a reference to the breakpoint_linux entity, and I think that breakpoint_elf can be declared static in cr16-tdep.c. > +/* Target-dependent structure in gdbarch. */ > +/* Architecture specific data. */ > +struct gdbarch_tdep One comment is enough :). And empty line between comment and type definition. > + /* The ELF header flags specify the multilib used. */ > + int elf_flags; > + > + const char *breakpoint; /* Breakpoint instruction. */ I'd rather you moved the last comment to just above the field declaration, for consistency. And the type should probably use gdb_bytes. -- Joel