Re: [PATCH] Add support for Xilinx MicroBlaze

[Eli Zaretskii <eliz at gnu dot org>]

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