This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]