From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16584 invoked by alias); 15 Nov 2012 17:43:23 -0000 Received: (qmail 16575 invoked by uid 22791); 15 Nov 2012 17:43:21 -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, 15 Nov 2012 17:43:16 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id AFC8C2E1A2; Thu, 15 Nov 2012 12:43:15 -0500 (EST) 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 tDuoIyEi5Aj6; Thu, 15 Nov 2012 12:43:15 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 565282E054; Thu, 15 Nov 2012 12:43:15 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 26D5BC87DF; Thu, 15 Nov 2012 09:43:13 -0800 (PST) Date: Thu, 15 Nov 2012 17:43: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: <20121115174313.GC3790@adacore.com> References: <507279C7.8080401@codesourcery.com> <20121022224107.GB3713@adacore.com> <20121023135502.GA3555@adacore.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-11/txt/msg00415.txt.bz2 > Additionally I have also attached the bfd hunk which moves the globals into > externs in the opcodes file. I will submit that to binutils once my gdb port > gets a go ahead. Both bfd and opcode bits needs to be approved before the GDB part can go ahead. > 2012-10-26 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. Below are my comments. They are mostly minor, so we should be pretty close to approval. The opcode and bfd bits are maintained by the binutils project, however, and will need to be approved there, and then checked in, before this patch can proceed. > +/* Target-dependent code for GNU/Linux on the Sitel CR16 processors. > + > + Copyright 2012 Free Software Foundation, Inc. Small nit: Can you add the "(C)" after Copyright, please? [in cr16-linux-tdep.c] > +#include "solib-svr4.h" Is this include really needed. This would be a little surprising considering that solib-svr4.o is not part of the gdb_target_obs. > +static const char * > +cr16_linux_register_name (struct gdbarch *gdbarch, int regnr) > +{ > + static const char *const reg_names[] = > + { [...] > + }; > + > + return reg_names[regnr]; Would you mind adding an assertion that REGNR is within acceptable bounds, please? This will trigger an internal error instead of a out-of-bound buffer access... You could also possibly add an assertion that the number of elements in your array is equal to CR16_LINUX_NUM_REGS, if that makes sense. One other possible suggestion is to move the array outside of the function, and to use a static assertion. The advantage, I believe, is that the assertion will be computed by the compiler, and fail at build time, rather than run time, if something every goes hinky. > --- ./gdb_src.orig/gdb/cr16-tdep.c 1970-01-01 05:30:00.000000000 +0530 > +++ ./gdb_src/gdb/cr16-tdep.c 2012-10-25 10:44:15.000000000 +0530 > +static const char * > +cr16_register_name (struct gdbarch *gdbarch, int regnr) > +{ > + static const char *const reg_names[] = > + { [...] > + }; > + > + return reg_names[regnr]; Same as above. > +} > + > + > +/* Implement the "register_type" gdbarch method. */ > + > +static struct type * > +cr16_register_type (struct gdbarch *gdbarch, int reg_nr) > +{ > + switch (reg_nr) > + { > + case CR16_PC_REGNUM: /* Note:PC in CR16 is of 24 bits. */ Can you add a space after the colon? > + case CR16_FP_REGNUM: /*Frame Pointer reg. */ > + case CR16_SP_REGNUM: /*Stack Pointer reg. */ Can you add a space after "/*", please? Although, FP an SP are so common, the comment is hardly useful - but not harmful. > + case SIM_CR16_ISP_REGNUM: > + case SIM_CR16_USP_REGNUM: > + case SIM_CR16_INTBASE_REGNUM: > + return builtin_type (gdbarch)->builtin_int32; > + break; > + > + case SIM_CR16_PSR_REGNUM: > + case SIM_CR16_CFG_REGNUM: > + return builtin_type (gdbarch)->builtin_uint32; > + break; Why not merge all blocks that return builtin_int32? Same for any other registers that return the same type (maybe less of an obvious suggestion). This are just suggestions, so please do not feel obigated to follow them. > + allWords = > + ((ULONGLONG) words[0] << 32) + ((unsigned long) words[1] << 16) + > + words[2]; Can you please void using the mixed-cap style for variables? For instance, use all_words, not allWords. Also, when breaking lines at a binary operator, you should break before the operator, not after. I think I understand the reason for the casts/promotion, but can we cast to the same type for both pieces? For long lines like these, the GNU Coding Standards require us to enclose the rhs expression into parentheses. The parentheses are superfluous, but help code formatters (or your editor's formatter). Thus: all_words = (((ULONGLONG) words[0] << 32) + ((ULONGLONG) words[1] << 16) + words[2]) > + else > + { > + break; /* Terminate the prologue scan. */ > + } Can you remove the extra braces? They are unnecessary, and GDB's style is to not use them in that case. Note that they would be used if you had to add a a comment. Thus... else break; /* Terminate. */ ... but also ... else { /* Terminate the loop because ... */ break; } > +static struct value * > +cr16_frame_prev_register (struct frame_info *this_frame, > + void **this_prologue_cache, int regnum) > +{ > + struct cr16_prologue *p = > + cr16_analyze_frame_prologue (this_frame, this_prologue_cache); > + CORE_ADDR frame_base = cr16_frame_base (this_frame, this_prologue_cache); > + int reg_size = register_size (get_frame_arch (this_frame), regnum); > + ULONGEST ra_prev; It looks like ra_prev could be declared in the only else if block where it is actually used. And reg_size appears to be unused. > + /* If prologue analysis says we saved this register somewhere, > + return a description of the stack slot holding it. */ > + else if (p->reg_offset[regnum] != 1) > + { > + return frame_unwind_got_memory (this_frame, regnum, > + frame_base + p->reg_offset[regnum]); > + } > + > + /* Otherwise, presume we haven't changed the value of this > + register, and get it from the next frame. */ > + else > + { > + return frame_unwind_got_register (this_frame, regnum, regnum); > + } Can you also remove the unnecessary curly braces above, please? > + CORE_ADDR pc; > + Trailing spaces here... > +static const gdb_byte * > +cr16_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR * pcptr, > + int *lenptr) [...] > + if (tdep == NULL || tdep->breakpoint == NULL) > + { > + return breakpoint_elf; > + } Unnecessary braces... > + /* Passing NULL values in the following two functions > + for the time being, to fix later. */ > + set_gdbarch_print_insn (gdbarch, print_insn_cr16); > + set_gdbarch_unwind_pc (gdbarch, cr16_unwind_pc); > + set_gdbarch_unwind_sp (gdbarch, cr16_unwind_sp); I don't understand the comment... > + /* Methods for saving / extracting a dummy frame's ID. > + The ID's stack address must match the SP value returned by > + PUSH_DUMMY_CALL, and saved by generic_save_dummy_frame_tos. */ The generic_save_dummy_frame_tos function was deleted in 2004... I suspect you copy/pasted this code from somewhere else. Fortunately, I think that's easily fixed, as I don't see the real benefit of the second sentence. The first one can be used as visual cue that the frame ID code starts there, but the second can just be deleted entirely. > --- ./gdb_src.orig/gdb/configure.tgt 2012-08-02 01:18:44.000000000 +0530 > +++ ./gdb_src/gdb/configure.tgt 2012-10-23 15:09:24.000000000 +0530 > --- ./gdb_src.orig/gdb/cr16-linux-tdep.c 1970-01-01 05:30:00.000000000 +0530 > +++ ./gdb_src/gdb/cr16-linux-tdep.c 2012-10-23 15:22:34.000000000 +0530 > --- ./gdb_src.orig/gdb/cr16-tdep.c 1970-01-01 05:30:00.000000000 +0530 > +++ ./gdb_src/gdb/cr16-tdep.c 2012-10-25 10:44:15.000000000 +0530 > --- ./gdb_src.orig/gdb/cr16-tdep.h 1970-01-01 05:30:00.000000000 +0530 > +++ ./gdb_src/gdb/cr16-tdep.h 2012-10-23 15:22:53.000000000 +0530 ? Looks like you have all these files twice in the patch you sent? I'll assume a pilot error an not review the second instances. -- Joel