Re: [PATCH] Add support for Xilinx MicroBlaze

Joel Brobecker
2009-09-26T00:08:20Z

> * 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.
> * NEWS: Announce Xilinx MicroBlaze support.

In the makefile, you'll need to add microblaze-tdep.h to the
HFILES_NO_SRCDIR list. Not strictly needed in order to build
GDB, but this is used by the developers who use ctags/etags.

> + Copyright 2009 Free Software Foundation, Inc.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 2 of the License, or
> + (at your option) any later version.

The copyright header is out of date. It should refer to version 3
of the GPL. Any C file in the GDB sources should have the correct
copyright header. Can you update all the new files that you are
submitting?

> + if ((val == 0) && (memcmp (bp, old_contents, bplen) == 0))

The extra parens should be removed. We discussed this with other
Global Maintainers, and we all felt that this difference in style
made the code harder to read, and thus should be avoided. So it
should be:

if (val == 0 && memcmp (bp, old_contents, bplen) == 0)

Can you fix the rest of the code accordingly?

> +static struct tramp_frame microblaze_linux_sighandler_tramp_frame = {

Just to be consistent, this curly brace should be on the next line.

> + gdbarch_register_osabi (bfd_arch_microblaze, 0,
> + GDB_OSABI_LINUX, microblaze_linux_init_abi);

Nit: formatting is wrong on the second line.

> +/* Picobug only supports a subset of registers from MCore. In reality,
> + it doesn't support ss1, either. */

Formatting nit: 2 spaces after the period.

> +void
> +picobug_open (char *args, int from_tty)

Can this be made static?

> +/* FIXME - this shouldn't be here */
> +#include "../opcodes/microblaze-opcm.h"

FIXME? If you remove the "../", I *think* it's fine to include
opcodes headers files from GDB code.

> +extern enum microblaze_instr get_insn_microblaze (long, bfd_boolean *,
> + enum microblaze_instr_type *, short *);
> +extern unsigned long microblaze_get_target_address (long, bfd_boolean, int, long, long,
> + long, bfd_boolean *, bfd_boolean *);
> +extern enum microblaze_instr microblaze_decode_insn (long, int *, int *,
> + int *, int *);

Can we avoid these extern declarations, though? It would be nice if
we could get a warning if the implementation of these functions changed.
This won't happen with these locally-defined externs.

> +#define mb_error(msg) internal_error (__FILE__, __LINE__, _(msg))

I don't like this define much, because an outsider inspecting part
of your code might not realize that mb_error is actually an internal
error. But if you prefer it that way...

> +static void
> +microblaze_dump_insn (char *commnt, CORE_ADDR pc, int insn)
> +{
> + if (microblaze_debug)
> + {
> + printf_filtered ("MICROBLAZE: %s %08x %08x ",
> + commnt, (unsigned int) pc, (unsigned int) insn);
> + /* print_insn_microblaze (pc, &tm_print_insn_info); */
> + printf_filtered ("\n");
> + }
> +}

Er, this function does nothing functional, whereas I was expecting
it to do something. But then, looking at the usage, it's just a debug
function. This reminds me that all new functions should be documented
with a comment at the beginning of their implementation. There are
exceptions, however; All the routines that implement gdbarch methods
or target methods are already documented as part of either gdbarch
or target. Same for solib vectors, etc. So you don't need to provide
the overal documentation for the functions used in your particular
target.

In this case, the general style is usually to conditionalize the call
to that function to microblaze_debug, rather than having the condition
inside the function. Some purists will say that this avoids a function
call if debug is off, but I'll just say that it makes the naming of that
function a littler easier :). If you prefer to keep the condition inside
the function, not a problem, but please consider choosing a name that
expresses this fact.

> +#define microblaze_insn_debug(args) \
> + { if (microblaze_debug) printf_filtered args; }

Perhaps if you added the braces around args in your call to
printf_filtered, you wouldn't need the extra parens when using
his macro?

> +#define mb_warn(msg) \
> + if (microblaze_debug) printf ("mb_warning: %s:%d %s\n", __FILE__, __LINE__, _(msg))
> +

Let's not use this home-made warning. See below.

> +unsigned long
> +microblaze_fetch_instruction (CORE_ADDR pc)
[...]
> + insn = 0;
> + for (i = 0; i < sizeof (buf); i++)
> + insn = (insn << 8) | buf[i];
> + return insn;

You can replace all this by a call to extract_unsigned_integer...

> + mb_warn ("push_dummy_code not implemented");
> + mb_warn ("store_arguments not implemented");

Please call "error" instead: The user will then be told that this
feature is not implemented, and the action that required this function
call will be automatically aborted as a result.

> +/* Return a pointer to a string of bytes that encode a breakpoint
> + instruction, store the length of the string in *LEN. */

As explained above, the comment is unnecessary; but if you decide
to keep it, there is just one nit: you need an extra space after
the period (I can reassure you that I make these mistakes myself,
so you're not the only one commented on these).

> +static const gdb_byte *
> +microblaze_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pc, int *len)

Formatting: This line is a little too long...

> + The prologue ends when an instruction fails to meet either of
> + these criteria. */
> +/* Analyze the prologue to determine where registers are saved,
> + the end of the prologue, etc. Return the address of the first line
> + of "real" code (i.e., the end of the prologue). */
> + /* Find the start of this function. */

Missing spaces after periods. There are other instances of this,
but I stopped pasting them here. Can you make sure you take care
of them too?

> + bfd_boolean save_hidden_pointer_found = FALSE;
> + bfd_boolean non_stack_instruction_found = FALSE;

We tend to prefer using integers for predicates.

> + cache->framesize = -1 * imm; /* stack grows towards low memory */

Can't we just do -imm?

> + /* FIXME: Rework this code and add comments. */

Please consider doing this now :)

> + default:
> + printf_filtered("Fatal error: unsupported return value size "
> + "requested (%s @ %d)\n", __FILE__, __LINE__);

This should be replaced by a call to "error".

> + Values less than 32 bits (short, boolean) are stored in r2, right
> + justified and sign or zero extended. FIXME

It would be nice to have the FIXME addressed now.

> +static int
> +microblaze_stabs_argument_has_addr (struct gdbarch *gdbarch, struct type *type)
> +{
> + return (TYPE_LENGTH (type) == 16); /* FIXME */

Can you take care of this FIXME as well? If you don't know whether
you have anything to fix or not, just put a note saying that, as far
as you know, the only matching case is when TYPE_LENGTH is 16.
I'm surprised you even need this at all. Are you using stabs?

> +microblaze_can_use_hardware_watchpoints (enum bptype type, int len, int ot)

I assume you have unlimited number of hardware breakpoint/watchpoints?
How nice.

> + bfd_boolean isunsignednum;
Can you rename this variable to is_unsingned_num? It's so much more
readable that way...

> + bfd_boolean unconditionalbranch;
> + microblaze_decode_insn(insn, &lrd, &lra, &lrb, &limm);

Can you use an empty line between the last declaration and the first
line of code?

> + {
> + stepbreaks[1].valid = TRUE;
> + }

Another small nit. We prefer not to use the curly braces if the
if block contains only one statement.

> +static int
> +microblaze_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int reg)
> +{
> + return dwarf2_to_reg_map[reg];

Can you add a call to gdb_assert that verifies that REG is within
the bounds of dwarf2_to_reg_map?

> +#undef MICROBLAZE_DEBUG
> +#ifdef MICROBLAZE_DEBUG
> +int microblaze_debug = 1;
> +#endif

You probably meant to remove this part.

> +#define RWSDP_REGNUM 13
> +#define LINK_REGNUM 15
> +#define PR_REGNUM 15
> +#define FIRST_ARGREG 5
> +#define LAST_ARGREG 10
> +#define RETVAL_REGNUM 3

Just a thought: Why not use the register-name litterals that you just
defined above? Wouldn't that be clearer?

> +/* All registers are 32 bits. */
> +#define REGISTER_SIZE 4

This is slightly paranoin, but we used to have a REGISTER_SIZE macro
that corresponded to a call to gdbarch_register_size. Since you are
defining this macro in a header file, can you rename it to
MICROBLAZE_REGISTER_SIZE? I just want to avoid the confusion as well
as avoid a clash with other parts of the code.

> +#define MY_FRAME_IN_SP 0x1
> +#define MY_FRAME_IN_FP 0x2

Can you either move these macros to microblaze-tdep.c or prefix
them with MICROBLAZE_? Note that you have a second identical
definition of these macros just a few lines below. The compiler
usually emits a warning about that...

> +#define NO_MORE_FRAMES 0x4

This one does not appear to be used anywhere. It also appears to be
duplicated.

> +#define IS_RETURN(op) (op == rtsd || op == rtid)
> +#define IS_UPDATE_SP(op, rd, ra) \
> + ((op == addik || op == addi) && rd == REG_SP && ra == REG_SP)
> +#define IS_SPILL_SP(op, rd, ra) \
> + ((op == swi || op == sw) && rd == REG_SP && ra == REG_SP)
> +#define IS_SPILL_REG(op, rd, ra) \
> + ((op == swi || op == sw) && rd != REG_SP && ra == REG_SP)
> +#define IS_ALSO_SPILL_REG(op, rd, ra, rb) \
> + ((op == swi || op == sw) && rd != REG_SP && ra == 0 && rb == REG_SP)
> +#define IS_SETUP_FP(op, ra, rb) \
> + ((op == add || op == addik || op == addk) && ra == REG_SP && rb == 0)
> +#define IS_SPILL_REG_FP(op, rd, ra, fpregnum) \
> + ((op == swi || op == sw) && rd != REG_SP && ra == fpregnum && ra != 0)
> +#define IS_SAVE_HIDDEN_PTR(op, rd, ra, rb) \
> + ((op == add || op == addik) && ra == FIRST_ARGREG && rb == 0)

Same for these, I think they should be moved to a .c file or
prefixed by MICROBLAZE_

> +/* BREAKPOINT defines the breakpoint that should be used. */
> +#ifdef __MICROBLAZE_UCLINUX__
> +#define BREAKPOINT {0xb9, 0xcc, 0x00, 0x60}
> +#else
> +#define BREAKPOINT {0x98, 0x0c, 0x00, 0x00}
> +#endif

Same remark about the name of the macro. But a bigger concern is
the fact that the macro is statically defined based on what I am
guessing is a host-specific macro. This is a target-specific file.
This goes against what we call our "multi-arch" design.

Is there no way for you to determine that at run time? For instance,
is there any microblaze-linux other than uclinux? If the breakpoint
instruction on linux is always {0xb9, 0xcc, 0x00, 0x60} while it is
{0x98, 0x0c, 0x00, 0x00} on the rest of the targets, that's easy to fix.
Otherwise, we'll have to come up with another form of detection.

--
Joel