This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: PATCH: new gdb port: moxie-elf


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  <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.

> +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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]