Re: [PATCH] Add support for Xilinx MicroBlaze

[Michael Eager <eager at eagercon dot com>]

Joel Brobecker wrote:

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

Actually, I meant to move these to a header file and include it
here and where the functions are defined.


> > +  /* FIXME:  Rework this code and add comments. */
>
> Please consider doing this now :)

Maybe, if I can figure out why the note was added and
what changes are needed.  :-\


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

Ditto.

> > +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?

It depends on whether users are still using older compilers (gcc-2.95 era)
which generated stabs.  I don't want to remove backward compatibility.
On the other hand, I don't want to spend any time and energy fixing
potential problems in code that has no future (or present).

> > +microblaze_can_use_hardware_watchpoints (enum bptype type, int len, int ot)
>
> I assume you have unlimited number of hardware breakpoint/watchpoints?
> How nice.

Infinite is always good.  I'll find out how many hw bp's actually exist.

 >> +/* 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.

Yes, there is MicroBlaze Linux with glibc, at least in development
if not currently available.  I'll have to find out if it uses the
same breakpoint; it probably does.  I'll see if I can come up with a
run-time test to distinguish embedded from hosted.

Thanks for your review and comments!



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