Re: Support for Xilinx MicroBlaze architecture (1 of 3)

[Joel Brobecker <brobecker@adacore.com>]

> 2009-09-11  Michael Eager <eager@eagercon.com>
>
> 	* configure.tgt:  Add targets microblaze*-linux-*, microblaze*-xilinx-*.
> 	* doc/gdb.texinfo: Add MicroBlaze.
> 	* MAINTAINERS: Add self as maintainer for MicroBlaze.
> 	* Makefile.in: Build microblaze-tdep.o, microblaze-linux-tdep.o.
[...]

It looks like this patch is against an older version of the debugger.
It makes references to entities or files that no longer exist. For
instance, it's references solib-legacy.c which was removed in 2007,
or BREAKPOINT_FROM_PC (use gdbarch_breakpoint_from_pc instead, since
2007 as well). Can you resync your patch against our CVS HEAD and
resubmit?

I did notice a couple of things while looking at the code:

> +/* See ppc_linux_memory_remove_breakpoints for description. */
> +int
> +microblaze_linux_memory_remove_breakpoint (struct bp_target_info *bp_tgt)
> +{

Because this is a target-independent implementation of a gdbarch
routine, you don't need to explain what this function does. You might
want to explain why it is necessary in your particular target, but
the comments inside the implementation seem to be clear enough as
to why the default implementation is not sufficient. So I would
drop the comment.

The other thing I noticed is that this function should be made static.

> +  const unsigned char *bp;

You should use gdb_byte

I only glanced at the rest of the code. I noticed commented out code
which should be removed (#if 0 ... #endif). There is also a commented
out printf. Formatting issues, such as missing double-space after a
period:

> +     then our frame has not yet been set up. */

(I am guilty of making these mistakes myself)

We prefer it if you have an empty line between variable declarations
and the rest of the code.

You don't need to protect your debugging traces against MICROBLAZE_DEBUG.
If these traces are useful to diagnose an issue, then have them available
through a "set/show debug " switch.

> +struct gdbarch_tdep
> +{
> +  int dummy;           /* declare something. */
> +};

I don't think you need to have any element in your tdep structure.

-- 
Joel