Re: Xtensa port

[Daniel Jacobowitz <drow@false.org>]

On Fri, Sep 22, 2006 at 11:25:12AM -0700, Maxim Grigoriev wrote:
> DESCRIPTION:
> 
>    Basic Xtensa port. It doesn't include any targets. The next step will be
> a submission of "xtensa*-*-linux" target. It will include gdbserver port and
> everything else necessary to debug Xtensa code on this target.
> 
> If this port is accepted Tensilica commits to maintain GDB Xtensa support.

Hi Maxim,

I've taken a quick look at this, and have just a few comments; I
will not have time to review it thoroughly for a while (though maybe
someone else will - Michael?).  Overall it looks nice.

The stuff I'm working on right now is data-driven (XML) GDB
descriptions of targets.  It seems like it would make this port
a whole lot simpler.  Unfortunately, while I plan to contribute
it ASAP, that still may be a few months.

> TESTING:
> 
>    Testing has been done on an Xtensa XT2000 evaluation board under 
> JTAG OCD control. GDB used GDB remote protocol to communicate through
> TCP/IP sockets with an OCD daemon controlling the board. All the
> changes to the DejaGnu testing frame and some generic GDB code
> updates are not part of this submission because this testing can not
> be reproduced outside Tensilica as of yet.

Does this mean that you need patches to more of GDB to use the code
that you're posting?

> +#include <xtensa-config.h>

This is supposed to come from the top level include/ directory, right?
In that case it should use "" quotes.

> +/* Check version of configuration file.  */
> +#define XTENSA_CONFIG_VERSION 0x60
> +#if XTENSA_TDEP_VERSION != XTENSA_CONFIG_VERSION
> +#warning "xtensa-config.c version mismatch!"
> +#endif

I gather this is a sample of a generated configuration file from
your tools?  Ugly...

> Index: gdb/xtensa-tdep.h
> ===================================================================
> @@ -0,0 +1,319 @@

> +static int xtensa_debug_level = 0;

This can't possibly belong in a header file if it's supposed to be
static.  Maybe this and the associated macros should be in the tdep.c
instead.

> +#define XTENSA_REMOTE_PACKET_SIZE (1000 - 1)

I wondered what this was for... in fact it seems to be unused.

> +/* Xtensa ELF core file register set representation ('.reg' section). 
> +   Copied from target-side ELF header <xtensa/elf.h>.  */
> +
> +typedef unsigned long elf_greg_t;

I recommend you not use this generic type name, since many system
headers use the same name; it'll just get you in trouble.

> +#define ELF_NGREG (SIZEOF_GREGSET / sizeof(elf_greg_t))

Similar for this.

> +/* get_fp_num() returns the register number of the frame pointer for the frame
> +   specified by pc.  Depending on how the function was compiled, the fp could
> +   be either a1 (sp) or another register set by the compiler.
> +   Note: it returns register number for an Ax, not for ARx.
> +
> +   We extract the FP register number from the DWARF info generated by the
> +   compiler.  If anything is wrong with the DWARF info or there is no DWARF
> +   info at all we return A1_REGNUM.  */
> +
> +static int
> +get_fp_num (CORE_ADDR pc)
> +{
> +  struct symtab_and_line debug_info;
> +  struct symbol *func_sym;
> +
> +  DEBUGTRACE ("get_fp_num (pc = 0x%08x)\n", (int) pc);
> +
> +  debug_info = find_pc_line (pc, 0);
> +  /* If there is no debug info return A1.  */
> +  if (debug_info.line == 0)
> +    return A1_REGNUM;
> +
> +  func_sym = find_pc_function (pc);
> +  if (func_sym)
> +    {
> +      if (SYMBOL_OPS (func_sym) == &dwarf2_locexpr_funcs)

No way.  I don't know what you need this information for, but we have
to find some way to get it that does not require grubbing around in the
private data structures of the symbol reader.

> +  /* Adjust the stack pointer and align it.  */
> +  sp = (sp - onstack_size) & ~(SP_ALIGNMENT - 1);

There's a separate gdbarch method for this nowadays.

> +  /* Renumber registers for known formats (stab, dwarf, and dwarf2).  */
> +  set_gdbarch_stab_reg_to_regnum (gdbarch, xtensa_reg_to_regnum);
> +  /* set_gdbarch_ecoff_reg_to_regnum (gdbarch, no_op_reg_to_regnum);  */
> +  set_gdbarch_dwarf_reg_to_regnum (gdbarch, xtensa_reg_to_regnum);
> +  /* set_gdbarch_sdb_reg_to_regnum (gdbarch, no_op_reg_to_regnum);  */
> +  set_gdbarch_dwarf2_reg_to_regnum (gdbarch, xtensa_reg_to_regnum);

Please delete commented out code.

-- 
Daniel Jacobowitz
CodeSourcery