From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3750 invoked by alias); 23 Apr 2009 18:29:07 -0000 Received: (qmail 3736 invoked by uid 22791); 23 Apr 2009 18:29:06 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00 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, 23 Apr 2009 18:28:57 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 7AD262BABAD; Thu, 23 Apr 2009 14:28:55 -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 8XnfT38pJQqw; Thu, 23 Apr 2009 14:28:55 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 419872BAB62; Thu, 23 Apr 2009 14:28:55 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id CD260F5924; Thu, 23 Apr 2009 11:28:51 -0700 (PDT) Date: Thu, 23 Apr 2009 18:29:00 -0000 From: Joel Brobecker To: green@moxielogic.com Cc: gdb-patches@sourceware.org Subject: Re: PATCH: new gdb port: moxie-elf Message-ID: <20090423182851.GC7552@adacore.com> References: <20090423045725.aaa2c6acbe2fcbd4897bea2c255aade5.61d9530215.wbe@email03.secureserver.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20090423045725.aaa2c6acbe2fcbd4897bea2c255aade5.61d9530215.wbe@email03.secureserver.net> User-Agent: Mutt/1.5.18 (2008-05-17) 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: 2009-04/txt/msg00655.txt.bz2 Hi Anthony, I'm a little unclear about the ownership of the patches you are submitting. I think I located you in the FSF assignment files, but it seems to be listing you as an individual instead of you as part of Moxie Logic. Is Moxie Logic a company that might have a claim on these changes? > 2009-04-23 Anthony Green > > * moxie-tdep.h: New file. > * moxie-tdep.c: New file. > * configure.tgt: Add moxie-elf. Overall, these look good to me. I do have a few comments, however. First, generally speaking, we would like you to provide a short description in the form of a comment at the beginning of every function. For the functions that implement gdbarch "methods", we would like a short comment explaining which gdbarch method the function implements. Please do not explain what the function does if it duplicates the gdbarch documentation. > +moxie-*-elf) > + gdb_target_obs="moxie-tdep.o" > + gdb_sim=../sim/moxie/libsim.a The second line means that you won't be able to build for moxie-elf unless the simulator part is approved and checked in. If getting the sim parts in takes a bit of time, you may elect to keep that line out of the patch, and resubmit it later, after the sim patch is in. > +const static unsigned char * > +moxie_breakpoint_from_pc (struct gdbarch *gdbarch, > + CORE_ADDR *pcptr, int *lenptr) > +{ > + static unsigned char breakpoint[] = { 0x35, 0x00 }; > + *lenptr = sizeof (breakpoint); > + return breakpoint; > +} The style in GDB is to skip a line after the variable declarations. For instance: static unsigned char breakpoint[] = { 0x35, 0x00 }; *lenptr = sizeof (breakpoint); > +/* Return the GDB type object for the "standard" data type > + of data in register N. */ This is an instance were the comment duplicates the gdbarch documentation. We'd like to avoid that. That way, if we were to ever change the specs of that method, we don't have to change that comment. Can you take care of all such comments? I can suggest, as an example: /* Implement the "register_type" gdbarch method. */ > + Things always get returned in RET1_REGNUM, RET2_REGNUM. */ Suggest embedding the comment inside the function body. But that's just a suggestion. If you prefer it in the function description, that's also fine. > +/* Function: moxie_scan_prologue I would prefer if you did not repeat the name of the function (I think BFD does that), especially when the name is wrong :-). > + /* Record where the jsra instruction saves the PC and FP */ I'm sorry to hit you with what really amounts to nitpicking on style issues, but the GNU Coding Style is specific on this, and once you've learnt it, it won't feel so painful. Period at the end of the sentence, please... > + for (next_addr = start_addr; > + next_addr < end_addr; ) Any reason why you decided to plit this over two lines? I'm pretty sure that gdb_indent.sh would join the two back into a single line... > + if (inst >= 0x0614 && inst <= 0x061f) /* push %r2 .. push %r13 */ I find the comment a little confusing. Does it mean "push %rN" where N is anywhere between 2 to 13? Not a huge deal, however. > + /* optional stack allocation for args and local vars <= 4 byte */ Style: Upper case letter at the beginning of the sentence, and period at the end. Thank you! > +/* Function: skip_prologue > + Find end of function prologue */ Same as before - let's not repeat the function name nor the gdbarch documentation. > +#define DEFAULT_SEARCH_LIMIT 128 Does not seem to be used anywhere... > +CORE_ADDR > +moxie_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc) I think this function can be made static. Also, I am wondering if you could rewrite the function using skip_prologue_using_sal - this function handles a lot of cases that your implementation doesn't. You can look at various implementations that use that function for inspiration on how they implemented it (rs6000-tdep, mips-tdep, for instance). > + /* Ignore return values more than 8 bytes in size because the moxie > + returns anything more than 8 bytes in the stack. */ Style: Missing space at the end of your sentence. > +static gdbarch_init_ftype moxie_gdbarch_init; Is that really necessary? Since your init function is defined before being used, I'd lose this declaration. > + /* Return the unwound PC value. */ This comment is unnecessary, IMO. -- Joel