Re: [PATCH] Add support for Xilinx MicroBlaze

Eli Zaretskii
2009-09-25T09:15:13Z

> Date: Wed, 23 Sep 2009 11:32:46 -0700
> From: Michael Eager <eager@eagercon.com>
>
> Attached is a revised patch which adds support for the
> Xilinx MicroBlaze. I believe that I've addressed all
> of the previous comments.

Thanks. And sorry for the delay ion reviewing your patch.

> * microblaze-linux-tdep.c: New.
> * microblaze-rom.c: New.
> * microblaze-tdep.c: New.
> * microblaze-tdep.h: New.

Please add the necessary tweaks for these new files to
gdb/config/djgpp/fnchange.lst. They will clash after 8+3 truncation.

> +@code{xmd} uses port @code{1234}. (While it is possible to change
^^
Two spaces between sentences, please.

> +@item target remote XMD-HOST:1234
> +Use this command to connect to the target if it is connected to @code{xmd}
> +running on a different system named @code{XMD-HOST}.

Please use "@var{xmd-host}" instead of a literal upper-case XMD-HOST.
@var is what should be used in Texinfo for identifiers that stand for
something else, in this case an actual host name.

> + /* printf("microblaze_analyze_prologue (pc = 0x%8.8x, "
> + "current_pc = 0x%8.8x, cache = 0x%8.8x)\n",
> + (int) pc, (int) current_pc, (int) cache); */

I believe we don't like dead code in GDB sources.

> + /* Spill register */

Comments should have a period and 2 blanks at their end.

> + cache->register_offsets[rd] = imm - cache->framesize;
> + /* reg spilled after updating */

Strange formatting of a comment, and indentation seems wrong.

> + /* We have a frame pointer. Note
> + the register which is acting as the frame pointer. */

Please reformat whitespace here: the first line is broken too early.

> + switch (TYPE_LENGTH(type))

I believe GNU standards call for a space before the left paren.

> + default:
> + printf_filtered("Fatal error: unsupported return value size "
> + "requested (%s @ %d)\n", __FILE__, __LINE__);

Shouldn't you call `fatal' here?

In any case, messages presented to the user should be in _(), to be
translatable (the debug messages are exempt from this rule).

> + /* Debug this files internals. */
> + add_setshow_zinteger_cmd ("microblaze", class_maintenance,
> + &microblaze_debug, _("\
> +Set microblaze debugging."), _("\
> +Show microblaze debugging."), _("\
> +When non-zero, microblaze specific debugging is enabled."),
> + NULL,
> + NULL,
> + &setdebuglist, &showdebuglist);

This command should be documented in the user manual.

> --- gdb/gdb/NEWS 2009-09-06 10:14:42.000000000 -0700
> +++ mb-gdb/gdb/NEWS 2009-09-23 10:41:55.000000000 -0700
> @@ -442,6 +442,11 @@ Lattice Mico32 lm32-*
> x86 DICOS i[34567]86-*-dicos*
> x86_64 DICOS x86_64-*-dicos*
> S+core 3 score-*-*
> +Xilinx MicroBlaze microblaze-*-*
> +
> +* New Simulators
> +
> +Xilinx MicroBlaze microblaze

The patch for NEWS is fine.

Thanks.