Re: [RFA 3/5] New port: CR16: gdb port

Yao Qi
2012-10-08T06:59:38Z

On 10/04/2012 06:20 PM, Kaushik Phatak wrote:

Hi, Kaushik,
I don't read your patch in details yet, just some small points I found...

> +const gdb_byte breakpoint_uclinux[] = { 0xC7, 0x00 };

Add 'static'. It is not used out of this file.

> +/* Allocate and initialize a gdbarch object. */
> +static struct gdbarch *
> +cr16_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> +{
> + struct gdbarch *gdbarch;
> + struct gdbarch_tdep *tdep;
> + int elf_flags;
> +
> + /* Extract the elf_flags if available. */
> + if (info.abfd != NULL
> + && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour)
> + elf_flags = elf_elfheader (info.abfd)->e_flags;
> + else
> + elf_flags = 0;
> +
> + /* Try to find the architecture in the list of already defined
> + architectures. */
> + for (arches = gdbarch_list_lookup_by_info (arches, &info);
> + arches != NULL;
> + arches = gdbarch_list_lookup_by_info (arches->next, &info))
> + {
> + if (gdbarch_tdep (arches->gdbarch)->elf_flags != elf_flags)
> + continue;
> +
> + return arches->gdbarch;
> + }
> + /* None found, create a new architecture from the information
> + provided. */
> + tdep = (struct gdbarch_tdep *) xcalloc (1,sizeof (struct gdbarch_tdep));
> + tdep->elf_flags = elf_flags;
> + gdbarch = gdbarch_alloc (&info, tdep);
> +
> + set_gdbarch_num_pseudo_regs (gdbarch, 0);
> + if(info.osabi == GDB_OSABI_LINUX)
> + {
> + set_gdbarch_num_regs (gdbarch, CR16_LINUX_NUM_REGS);
> + set_gdbarch_register_name (gdbarch, cr16_linux_register_name);

This part should be moved to function cr16_uclinux_init_abi, and then
macro CR16_LINUX_NUM_REGS can be defined in cr16-linux-tdep.c instead of
cr16-tdep.h.

> + }
> + else
> + {
> + set_gdbarch_num_regs (gdbarch, CR16_NUM_REGS);
> + set_gdbarch_register_name (gdbarch, cr16_register_name);
> + }

> --- ./gdb_src.orig/gdb/cr16-tdep.h 1970-01-01 05:30:00.000000000 +0530
> +++ ./gdb_src/gdb/cr16-tdep.h 2012-09-04 13:05:39.000000000 +0530
> @@ -0,0 +1,36 @@
> +/* GNU/Linux on CR16 target support.
> + Copyright (C) 2011-2012 Free Software Foundation, Inc.
> +
> + Contributed by Kaushik Phatak (kaushik.pahatk@kpitcummins.com)
> + KPIT Cummins Infosystems Limited, Pune India.
> +
> + 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 3 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, see<http://www.gnu.org/licenses/>. */
> +
> +#define CR16_NUM_REGS 22
> +#define CR16_LINUX_NUM_REGS 21

These two macros can be defined in cr16-tdep.c and cr16-linux-tdep.c
respectively.

> +
> +extern const gdb_byte breakpoint_elf[];
> +extern const gdb_byte breakpoint_linux[];

They are not needed. 'breakpoint_elf' is only used in cr16-tdep.c, and
'breakpoint_linux' doesn't exist at all.

--
Yao