Re: [PATCH] Support for Xilinx MicroBlaze

[Joel Brobecker <brobecker at adacore dot com>]

> 2009-10-05  Michael Eager <eager@eagercon.com>
>
>   * config/djgpp/fnchange.lst: Add translations for cpu-microblaze.c,
>   elf32-microblaze.c, microblaze-rom.c, microblaze-linux-tdep.c,
>   microblaze-tdep.h, microblaze-tdep.c, microblaze-opc.h, microblaze-opcm.h,
>   microblaze-dis.c, microblaze-dis.h, sim/microblaze, microblaze.h, and
>   microblaze.isa.
>   * 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.
>   HFILES_NO_SRCDIR: Add microblaze-tdep.h.
>   * microblaze-linux-tdep.c: New.
>   * microblaze-rom.c: New.
>   * microblaze-tdep.c: New.
>   * microblaze-tdep.h: New.
>   * NEWS: Announce Xilinx MicroBlaze support.

Overall, this looks like really good work. I just have a few comments,
and I think that the next iteration will be ready to be checked in.
I'll try to review the next revision faster, althought I will be away
for 10 days on a business trip next week.

The indentation for the ChangeLog entry needs to be 8 character, and
that will cause some of the lines to exceed 80 chars (I even prefer
limiting this to something less, such as 78 characters for instance).

As discussed privately, can you hold off on the following change
in MAINTAINERS:
> +			Michael Eager		eager@eagercon.com

For the record, this is not because we refuse to acknowledge Michael's
contribution of commitment to the port, but simply because this file
is used to list the port maintainers that have been nominated by the
GDB Global Maintainers.  See for instance the recent nomination of Jan
Kratochvil and Tristan Gingold.

> +		  if (picobug_regnames[i]
> +		      && (strcmp (picobug_regnames[i], name) == 0))

Just a nit, you have some unnecessary parens around strcmp (...) == 0.

> +static CORE_ADDR
> +microblaze_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
> +			    CORE_ADDR funcaddr,
> +			    struct value **args, int nargs,
> +			    struct type *value_type,
> +			    CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
> +			 struct regcache *regcache)

The formatting of the last line is slightly off...

> +  microblaze_insn_debug (("MICROBLAZE: Scanning prologue: name=%s, "
> +			 "func_addr=0x%x, stop=0x%x\n",
> +			 name, (unsigned int) func_addr,
> +			 (unsigned int) stop));

The extra parens are because the macro is defined without the parens
in the call to printf:

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

I would add the parens in the macro definition in order to make
the call to the macro more natural.  I also recommend, for debugging
traces, that you use printf_unfiltered.  Having to hit enter at every
new page when possibly generating tons of debugging traces can drive
someone crazy :).

Speaking of these debug traces, the macro name seems unnecessarily
specialized, as it really is nothing more than a printf.  In fact,
you do not use it just a few lines below, but you still output the
same type of trace with the same "MICROBLAZE: ..." format.  What
I personally do is define a ..._debug routine with a printf format
such as:

    void wtx_opt_objfiles_debug (const char *format, ...)
      ATTR_FORMAT (printf, 1, 2);

And with the following implementation:

    void
    wtx_opt_objfiles_debug (const char *format, ...)
    {
      if (debug_objfiles)
        {
          va_list args;
    
          va_start (args, format);
          printf_filtered ("DEBUG (objfiles): ");
          vprintf_filtered (format, args);
          printf_filtered ("\n");
          va_end (args);
        }
    }

(Gaaahhh, what did I just say about using printf_filtered).
I like this approach, even though it's slightly more code, simply
because it allows me to have consistent debug traces without having
to test my debug flag. For instance, the following would become:

> +      if (microblaze_debug) 
> +	printf_filtered ("MICROBLAZE: %08x %08lx\n", (unsigned int) pc, insn);

        microblaze_debug ("%08x %08lx\n", (unsigned int) pc, insn);

Actually, there is a routine to print CORE_ADDR values: paddress.
So it should be:

        microblaze_debug ("%s %08lx\n", paddress (pc), insn);

> +	  cache->register_offsets[rd] = imm - cache->framesize;
> +		/* reg spilled after updating.  */

Comments in GDB are placed before the associated code (unfortunately,
if you ask me).

> +	  microblaze_insn_debug (
> +		("MICROBLAZE: swi %d %d %d, continuing\n", rd, ra, imm));

It would look better if the parens on the first line did not terminate
the line.  This is just a general remark, as you won't need 2 parens
if you fix the macro or adopt the ..._debug function or macro as I
suggest above.

> +	if (!(op == 0x26 || op == 0x27 || op == 0x2d || op == 0x2e || op == 0x2f))

You may prefer to use

    if (op != 0x26 && op != 0x27 && ..)

instead of "if (!(op == 0x26 || op == 0x27 || ...))" which I find slightly
confusing because you have to remember to keep reading until you find the
end of the expression to which the "!" applies.  But this is a matter of
personal taste, so feel free to chose the version you prefer.

> +  /* For sentinel frame, return address is actual PC.  For other frames,
> +     return address is pc+8.  This is a workaround because gcc does not
> +     generate correct return address in CIE.  */

You do what you have to do, but this is *BAD* idea (IMO). Unless you can
detect the case when GCC generates bad return addresses or not in CIE,
you'll end up having a broken debugger as soon as the compiler gets
fixed.  Introducing work arounds for compiler deficiencies is often fine,
but I don't think that this should be done at the cost of proper operation.

> +      case 1:	/* return last byte in the register.  */
> +	regcache_cooked_read (regcache, MICROBLAZE_RETVAL_REGNUM, buf);
> +	memcpy(valbuf, buf + MICROBLAZE_REGISTER_SIZE - 1, 1);
> +	return;
> +      case 2:	/* return last 2 bytes in register.  */
> +	memcpy(valbuf, buf + MICROBLAZE_REGISTER_SIZE - 2, 2);
> +	return;
> +      case 4:	/* for sizes 4 or 8, copy the required length.  */
> +      case 8:
> +	regcache_cooked_read (regcache, MICROBLAZE_RETVAL_REGNUM, buf);
> +	regcache_cooked_read (regcache, MICROBLAZE_RETVAL_REGNUM+1, buf+4);
> +	memcpy (valbuf, buf, TYPE_LENGTH (type));
> +	return;
> +      default:
> +	fatal (_("Unsupported return value size requested"));

fatal is verbotten except in a few specific cases. Can you use
internal_error instead?

> +int
> +microblaze_can_use_hardware_watchpoints (enum bptype type, int count, int ot)
This function appears to be unused? Let's delete it if that is the case.

> +int
> +microblaze_software_single_step (struct frame_info *frame)

Can this function be made static?

> +  struct regcache *regcache = get_current_regcache ();

This one raised a red flag, as we try to avoid depending on global
variables.  But I'm not sure what the kosher way of getting the regcache
would be. I thought there would be method to get the regcache from
a frame, but apparently not.  Perhaps the right way is to use
get_thread_arch_regcache (inferior_ptid, gdbarch), but I'm not sure.
I'll ask Ulrich, who knows this area a lot better.

> +int
> +microblaze_frame_num_args_p (struct gdbarch *gdbarch)

Looks unused...

> +  /* Debug this files internals. */

Tiny nit: Missing second space after the period.

> +extern CORE_ADDR microblaze_analyze_prologue (CORE_ADDR, CORE_ADDR, 
> +					      struct microblaze_frame_cache *);

Let's make that function static and remove that declaration.

-- 
Joel