Re: [RFA][3/5] New port: Cell BE SPU (the port itself)

Mark Kettenis
2006-11-11T21:19:20Z

> Hello,
>
> this is the bulk of the new SPU port. It adds a configuration for
> spu*-*-* as target, as well as a 'pseudo-native' configuration hosted
> on powerpc that allows debugging SPU code natively on a Cell BE system.
>
> OK?

Hi Ulrich,

A couple of points:

First, "spu" doesn't occur anywhere in config.guess. Is this a name the
community agrees on? I understand it stands for Synergistic Processor Unit,
and it seems a bad idea to me that's a fairly generic term.

> * config/spu/spu-cell.mt: New file.
> * config/spu/spu.mt: New file.

Two .mt files? I think spu-cell.mt should be renamed spu.mh.

> * spu-nat.c: New file.

This file seems to be Linux-spefic. Can you rename it to spu-linux-nat.c?

> +/* Some older glibc versions do not define this. */
> +#ifndef __WNOTHREAD
> +#define __WNOTHREAD 0x20000000 /* Don't wait on children of
> other
> + threads in this group */
> +#endif

Is this really needed? I mean, sometimes stuff like this gets added when
we're working on getting anew platform running, but things get fixed
before there's an "offical" release.

> + sprintf (mess, "reading PPC register #%d", regno);

Can you use xsnprintf() wherever you use sprintf? The later is a very
dangerous function, and really should not be used in new code.

> + char buf[8];

Lots of places use char where you should use gdb_byte.

> diff -urN gdb-orig/gdb/spu-tdep.h gdb-head/gdb/spu-tdep.h
> --- gdb-orig/gdb/spu-tdep.h 1970-01-01 01:00:00.000000000 +0100
> +++ gdb-head/gdb/spu-tdep.h 2006-11-10 02:10:48.572865680 +0100
> @@ -0,0 +1,44 @@
> +/* SPU target-dependent code for GDB, the GNU debugger.
> + Copyright (C) 2006 Free Software Foundation, Inc.
> +
> + This file is part of GDB.
> +
> + 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.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program; if not, write to the Free Software
> + Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + Boston, MA 02110-1301, USA. */
> +
> +#ifndef SPU_TDEP_H
> +#define SPU_TDEP_H
> +
> +/* Number of registers. */
> +#define SPU_NUM_REGS 130
> +#define SPU_NUM_PSEUDO_REGS 1
> +#define SPU_NUM_CORE_REGS 128
> +
> +/* SPU calling convention. */
> +#define SPU_LR_REGNUM 0
> +#define SPU_RAW_SP_REGNUM 1
> +#define SPU_ARG1_REGNUM 3
> +#define SPU_ARGN_REGNUM 79
> +#define SPU_FP_REGNUM 127
> +
> +/* Special registers. */
> +#define SPU_ID_REGNUM 128
> +#define SPU_PC_REGNUM 129
> +#define SPU_SP_REGNUM 130
> +
> +/* Local store. */
> +#define SPU_LS_SIZE 0x40000
> +
> +#endif

Many ports use an enum for the register numbers, which makes debugging a
bit easier. I also think SPU_NUM_CORE_REGS is a bad name. I first thought
this had something to do with core files. May I suggest SPU_NUM_VEC_REGS.

Mark