Support for Xilinx MicroBlaze architecture (1 of 3)

Support for Xilinx MicroBlaze architecture (1 of 3)

[Michael Eager]

[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]

Hi --

The attached patch adds support for the Xilinx MicroBlaze architecture.

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.
	* microblaze-linux-tdep.c: New.
	* microblaze-rom.c: New.
	* microblaze-tdep.c: New.
	* microblaze-tdep.h: New.
	* opcodes/microblaze-dis.c: Rewrite.
	* opcodes/microblaze-opc.h: Rewrite.
	* opcodes/microblaze-opcm.h: Rewrite.
	* sim/configure.ac: Add target microblaze-*-*.
	* sim/configure: Regenerate.
	* sim/microblaze/config.in: New.
	* sim/microblaze/configure.ac: New.
	* sim/microblaze/configure: Generate.
	* sim/microblaze/interp.c: New.
	* sim/microblaze/Makefile.in: New.
	* sim/microblaze/microblaze.h: New.
	* sim/microblaze/microblaze.isa: New.
	* sim/microblaze/sim-main.h: New.
	* sim/microblaze/sysdep.h: New.

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


[-- Attachment #2: gdb-1.patch.bz2 --]
[-- Type: application/x-bzip2, Size: 23527 bytes --]

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

[Eli Zaretskii]

> Date: Fri, 11 Sep 2009 12:15:27 -0700
> From: Michael Eager <eager@eagercon.com>
> 
> The attached patch adds support for the Xilinx MicroBlaze architecture.

Thanks.

I have a few comments for the documentation part of the patch:

>                                                       By default
> +@code{xmd} uses port @code{1234}.

Is there a way to change this default, and if so, should we let the
user know how to do that?

> +@item target remote XMD-HOST:1234
> +@kindex target remote xmd-host>:1234
> +Use this command to connect to the target if you are running @value{GDBN}
> +on a different system @code{xmd}, which is running on @code{XMD-HOST}.

I understand that XMD-HOST stands for the real name of the host.  If
so, we use the @var markup in Texinfo for this:

  +@item target remote @var{xmd-host}:1234
  +Use this command to connect to the target if you are running @value{GDBN}
  +on a different system @code{xmd}, which is running on @var{xmd-host}.

Finally, the index entries you have are not useful: they all are for
commands that are not specific to the XMD target, and already have
index entries elsewhere in the manual, where the general form of these
commands is described.  What is needed in this section is @cindex
entries about the XMD itself, so that the reader could easily find
this section.  So I suggest these entries immediately after the
@subsection line:

  @cindex Xilinx MicroBlaze
  @cindex XMD, Xilinx Microprocessor Debugger

Okay with these changes.

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

[Michael Eager]

Eli Zaretskii wrote:
>>                                                       By default
>> +@code{xmd} uses port @code{1234}.
> 
> Is there a way to change this default, and if so, should we let the
> user know how to do that?

There's documentation in the Xilinx EDK/SDK or running "xmd --help"
lists the options.  I believe that there is an option to set the port.

Do you think that xmd should be described in the gdb info?  Or
only this option?

> 
>> +@item target remote XMD-HOST:1234
>> +@kindex target remote xmd-host>:1234
>> +Use this command to connect to the target if you are running @value{GDBN}
>> +on a different system @code{xmd}, which is running on @code{XMD-HOST}.
> 
> I understand that XMD-HOST stands for the real name of the host.  If
> so, we use the @var markup in Texinfo for this:
> 
>   +@item target remote @var{xmd-host}:1234
>   +Use this command to connect to the target if you are running @value{GDBN}
>   +on a different system @code{xmd}, which is running on @var{xmd-host}.

Sure.

> Finally, the index entries you have are not useful: they all are for
> commands that are not specific to the XMD target, and already have
> index entries elsewhere in the manual, where the general form of these
> commands is described.  What is needed in this section is @cindex
> entries about the XMD itself, so that the reader could easily find
> this section.  So I suggest these entries immediately after the
> @subsection line:
> 
>   @cindex Xilinx MicroBlaze
>   @cindex XMD, Xilinx Microprocessor Debugger

I'll pull the generic index entries and add these.

> Okay with these changes.


Thanks.


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

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

[Eli Zaretskii]

> Date: Sat, 12 Sep 2009 11:35:26 -0700
> From: Michael Eager <eager@eagercon.com>
> CC: gdb-patches@sourceware.org
> 
> Eli Zaretskii wrote:
> >>                                                       By default
> >> +@code{xmd} uses port @code{1234}.
> > 
> > Is there a way to change this default, and if so, should we let the
> > user know how to do that?
> 
> There's documentation in the Xilinx EDK/SDK or running "xmd --help"
> lists the options.  I believe that there is an option to set the port.
> 
> Do you think that xmd should be described in the gdb info?  Or
> only this option?

Only that option would be enough.  In fact, it's even enough to say
"See the Xilinx EDK/SDK documentation or run @kbd{xmd --help}, for how
to set the port number."

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

[Eli Zaretskii]

> Date: Fri, 11 Sep 2009 12:15:27 -0700
> From: Michael Eager <eager@eagercon.com>
> 
> The attached patch adds support for the Xilinx MicroBlaze architecture.
> 
> 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.
> 	* microblaze-linux-tdep.c: New.
> 	* microblaze-rom.c: New.
> 	* microblaze-tdep.c: New.
> 	* microblaze-tdep.h: New.
> 	* opcodes/microblaze-dis.c: Rewrite.
> 	* opcodes/microblaze-opc.h: Rewrite.
> 	* opcodes/microblaze-opcm.h: Rewrite.
> 	* sim/configure.ac: Add target microblaze-*-*.
> 	* sim/configure: Regenerate.
> 	* sim/microblaze/config.in: New.
> 	* sim/microblaze/configure.ac: New.
> 	* sim/microblaze/configure: Generate.
> 	* sim/microblaze/interp.c: New.
> 	* sim/microblaze/Makefile.in: New.
> 	* sim/microblaze/microblaze.h: New.
> 	* sim/microblaze/microblaze.isa: New.
> 	* sim/microblaze/sim-main.h: New.
> 	* sim/microblaze/sysdep.h: New.

If this is accepted, we will need a short NEWS entry for this new
feature.

Thanks.

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

[Michael Eager]

Eli Zaretskii wrote:
>> Date: Fri, 11 Sep 2009 12:15:27 -0700
>> From: Michael Eager <eager@eagercon.com>
>>
>> The attached patch adds support for the Xilinx MicroBlaze architecture.
>>
>> 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.
>> 	* microblaze-linux-tdep.c: New.
>> 	* microblaze-rom.c: New.
>> 	* microblaze-tdep.c: New.
>> 	* microblaze-tdep.h: New.
>> 	* opcodes/microblaze-dis.c: Rewrite.
>> 	* opcodes/microblaze-opc.h: Rewrite.
>> 	* opcodes/microblaze-opcm.h: Rewrite.
>> 	* sim/configure.ac: Add target microblaze-*-*.
>> 	* sim/configure: Regenerate.
>> 	* sim/microblaze/config.in: New.
>> 	* sim/microblaze/configure.ac: New.
>> 	* sim/microblaze/configure: Generate.
>> 	* sim/microblaze/interp.c: New.
>> 	* sim/microblaze/Makefile.in: New.
>> 	* sim/microblaze/microblaze.h: New.
>> 	* sim/microblaze/microblaze.isa: New.
>> 	* sim/microblaze/sim-main.h: New.
>> 	* sim/microblaze/sysdep.h: New.
> 
> If this is accepted, we will need a short NEWS entry for this new
> feature.

OK.

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

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

[Joel Brobecker]

Michael,

> 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.
> 	* microblaze-linux-tdep.c: New.
> 	* microblaze-rom.c: New.
> 	* microblaze-tdep.c: New.
> 	* microblaze-tdep.h: New.

I will look at this part.

> 	* opcodes/microblaze-dis.c: Rewrite.
> 	* opcodes/microblaze-opc.h: Rewrite.
> 	* opcodes/microblaze-opcm.h: Rewrite.

This part is maintained by binutils. Can you send the patch there?

> 	* sim/configure.ac: Add target microblaze-*-*.
> 	* sim/configure: Regenerate.
> 	* sim/microblaze/config.in: New.
> 	* sim/microblaze/configure.ac: New.
> 	* sim/microblaze/configure: Generate.
> 	* sim/microblaze/interp.c: New.
> 	* sim/microblaze/Makefile.in: New.
> 	* sim/microblaze/microblaze.h: New.
> 	* sim/microblaze/microblaze.isa: New.
> 	* sim/microblaze/sim-main.h: New.
> 	* sim/microblaze/sysdep.h: New.

This part should be independent from the GDB part. Can you separate
it out and copy Ben Elliston and Frank Ch. Eigler, asking them for
review?

Thanks,
-- 
Joel

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

[Michael Eager]

Joel Brobecker wrote:
> Michael,
> 
>> 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.
>> 	* microblaze-linux-tdep.c: New.
>> 	* microblaze-rom.c: New.
>> 	* microblaze-tdep.c: New.
>> 	* microblaze-tdep.h: New.
> 
> I will look at this part.
> 
>> 	* opcodes/microblaze-dis.c: Rewrite.
>> 	* opcodes/microblaze-opc.h: Rewrite.
>> 	* opcodes/microblaze-opcm.h: Rewrite.
> 
> This part is maintained by binutils. Can you send the patch there?

Yes.

> 
>> 	* sim/configure.ac: Add target microblaze-*-*.
>> 	* sim/configure: Regenerate.
>> 	* sim/microblaze/config.in: New.
>> 	* sim/microblaze/configure.ac: New.
>> 	* sim/microblaze/configure: Generate.
>> 	* sim/microblaze/interp.c: New.
>> 	* sim/microblaze/Makefile.in: New.
>> 	* sim/microblaze/microblaze.h: New.
>> 	* sim/microblaze/microblaze.isa: New.
>> 	* sim/microblaze/sim-main.h: New.
>> 	* sim/microblaze/sysdep.h: New.
> 
> This part should be independent from the GDB part. Can you separate
> it out and copy Ben Elliston and Frank Ch. Eigler, asking them for
> review?

OK.


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

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

[Joel Brobecker]

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

-- 
Joel

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

[Michael Eager]

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