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

[Michael Eager <eager@eagercon.com>]

Joel Brobecker wrote:
>> 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.

Thanks.  I'll take a look at these issues.

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