From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19131 invoked by alias); 23 Apr 2009 21:25:03 -0000 Received: (qmail 19028 invoked by uid 22791); 23 Apr 2009 21:25:00 -0000 X-SWARE-Spam-Status: No, hits=-0.1 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from smtpauth05.prod.mesa1.secureserver.net (HELO smtpauth05.prod.mesa1.secureserver.net) (64.202.165.99) by sourceware.org (qpsmtpd/0.43rc1) with SMTP; Thu, 23 Apr 2009 21:24:50 +0000 Received: (qmail 18237 invoked from network); 23 Apr 2009 21:24:46 -0000 Received: from unknown (64.202.165.99) by smtpauth05.prod.mesa1.secureserver.net (64.202.165.99) with ESMTP; 23 Apr 2009 21:24:46 -0000 Message-ID: <49F0DC9C.3000802@moxielogic.com> Date: Thu, 23 Apr 2009 21:25:00 -0000 From: Anthony Green User-Agent: Thunderbird 2.0.0.21 (Windows/20090302) MIME-Version: 1.0 To: Joel Brobecker CC: gdb-patches@sourceware.org Subject: Re: PATCH: new gdb port: moxie-elf References: <20090423045725.aaa2c6acbe2fcbd4897bea2c255aade5.61d9530215.wbe@email03.secureserver.net> <20090423182851.GC7552@adacore.com> In-Reply-To: <20090423182851.GC7552@adacore.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed 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: 2009-04/txt/msg00666.txt.bz2 Joel Brobecker wrote: > 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? > No, moxie logic is the project umbrella name. There is no company. >> 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. > > Ok. >> +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. > > Good idea. >> +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); > > Ok. >> +/* 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. */ > > Ok. >> + 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 :-). > > Ok. >> + /* 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... > > Ok. >> + 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... > > No reason. I'll run gdb_indent.sh as well. >> + 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. > That's right. I'll make this more clear. > >> + /* 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! > Ok. > >> +/* 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... > > Oops. >> +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). > Thanks - I'll look at those. > >> + /* 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. > > Ok. >> + /* Return the unwound PC value. */ >> > > This comment is unnecessary, IMO. > > Thanks for taking the time to review, Joel. I'll make the suggested changes and resubmit. AG