Re: PATCH: new gdb port: moxie-elf

[Anthony Green <green@moxielogic.com>]

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  <green@moxielogic.com>                       
>>       
>>        * 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