Re: [PATCH v4] gdb: bfin: new port

[Pedro Alves <pedro at codesourcery dot com>]

On Wednesday 15 December 2010 16:47:25, Mike Frysinger wrote:
> Signed-off-by: Jie Zhang <jie.zhang@analog.com>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> 
> 2010-12-15  Jie Zhang  <jie.zhang@analog.com>
> 

Should really be, IMO:

2010-12-15  Jie Zhang  <jie.zhang@analog.com>
	Mike Frysinger <vapier@gentoo.org>

> +bfin-*-*)
> +       # Target: Blackfin
> +       gdb_target_obs="bfin-tdep.o bfin-linux-tdep.o linux-tdep.o"
> +       ;;

should probably be:

bfin-*-uclinux*)
       # Target: Blackfin running uclinux
       gdb_target_obs="bfin-tdep.o bfin-linux-tdep.o linux-tdep.o"
       ;;
bfin-*-*)
       # Target: Blackfin
       gdb_target_obs="bfin-tdep.o"
       ;;

The top-level src/configure.ac seems to actually use
bfin*-*-uclinux* (extra star).

You should update the triplets in the NEWS entry as well.

> +void
> +_initialize_bfin_tdep (void)

Needs a prototype:

/* Provide a prototype to silence -Wmissing-prototypes.  */
extern initialize_file_ftype _initialize_bfin_tdep;

There's the equivalent one in bfin-linux-tdep.c, but you're missing it
in bfin-tdep.c.

> +         warning (_("Function Prologue not recognised; pc will point to ENTRY_POINT of the function"));

way too long line.  write as, e.g., 

         warning (_("Function Prologue not recognised; "
                    "pc will point to ENTRY_POINT of the function"));

> +  /* initialize R0, R1 and R2 to the first 3 words of paramters */

 /* Initialize R0, R1 and R2 to the first 3 words of paramters.  */

Capitalize, typo, period, double space.

> +/* Target-dependent code for Analog Devices Blackfin processer, for GDB.

typo: processor

> +   Copyright (C) 2005-2010 Free Software Foundation, Inc.

I don't think we can use an year range in our sources:

<http://www.gnu.org/prep/maintain/maintain.html#Copyright-Notices>:

"You can use a range (â2008-2010â) instead of listing individual
years (â2008, 2009, 2010â) if and only if: 1) every year in the range, inclusive,
really is a âcopyrightableâ year that would be listed individually; and 2) you
make an explicit statement in a âREADMEâ file about this usage. "

I think we fail point #2.  I see no other use of an year range
in the sources.

b/gdb/bfin-tdep.h:
> +/* in opcodes/bfin-dis.c */
> +extern int print_insn_bfin (bfd_vma pc, struct disassemble_info *outf);

Please remove this.  It is declared in src/include/dis-asm.h.

Otherwise, with the astat/cc change, this looks good to me.

-- 
Pedro Alves