Re: [PATCH] Add support for Xilinx MicroBlaze

Michael Eager
2009-09-25T19:16:43Z

Eli Zaretskii wrote:
>> 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.

Thanks for the review. I'll fix these issues and resubmit.
Sorry this is taking so much back-and-forth.

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