Re: [PATCH] Add support for Xilinx MicroBlaze

[Michael Eager <eager@eagercon.com>]

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


-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077