Re: [PATCH][SH] Implement core-file support for sh-linux

[Joel Brobecker <brobecker@adacore.com>]

On Mon, Oct 19, 2009 at 06:26:51PM +0100, Andrew Stubbs wrote:
> 2009-10-19  Andrew Stubbs  <ams@codesourcery.com>
> 	    Joel Brobecker  <brobecker@adacore.com>
> 
> 	* configure.tgt (sh*-*-linux*): Add corelow.o to gdb_target_obs.
> 	* sh-linux-tdep.c: Include sh-tdep.h.
> 	(REGSx16): New macro.
> 	(gregs_table, fpregs_table): New variables.
> 	(sh_linux_init_abi): Set core_gregmap and fpregmap.
> 	* sh-tdep.c: Include regset.h.
> 	(sh_corefile_supply_regset): New function.
> 	(sh_corefile_collect_regset): New function.
> 	(sh_corefile_gregset, sh_corefile_fpregset): New variables.
> 	(sh_regset_from_core_section): New function.
> 	(sh_gdbarch_init): Set up tdep value.
> 	Call set_gdbarch_regset_from_core_section.
> 	* sh-tdep.h (PC_REGNUM): New enum value.
> 	(struct sh_corefile_regs): New type.
> 	(sh_corefile_gregset): Export variable.
> 	* shnbsd-tdep.c: Remove include of regcache.h and gdb_assert.h.
> 	(regmap): Use new definition using struct sh_corefile_regs.
> 	(shnbsd_supply_gregset, shnbsd_collect_gregset): Delete.
> 	(shnbsd_gregset): Delete.
> 	(shnbsd_regset_from_core_section): Delete.
> 	(shnbsd_supply_reg, shnbsd_fill_reg): Use new regset interface.
> 	(shnbsd_init_abi): Set core_gregmap.

Thanks!

I just a few minor comments. This is pre-approved after the comments
have been addressed.

> +  /* Core files are supported for 32-bit SH only, at present.  */
> +  if (info.bfd_arch_info->mach != bfd_mach_sh5)
> +    {
> +      struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +      tdep->core_gregmap = (struct sh_corefile_regmap *)gregs_table;

Can you add en empty line between the declarations and the first
statement?  This is something that some of the maintainers insist on,
so let's try to be as consistent as we can.

> +  const struct sh_corefile_regmap *regmap = regset == &sh_corefile_gregset
> +					    ? tdep->core_gregmap
> +					    : tdep->core_fpregmap;

Just another formatting nit: the Gnu Coding Standards actually explicitly
help in that case and ask that we use parens around your expression.
This helps gnu indent as well as emacs format your code properly, but
I just think it makes it more readable.

const struct sh_corefile_regmap *regmap = (regset == &sh_corefile_gregset
                                           ? tdep->core_gregmap
                                           : tdep->core_fpregmap);

Likewise in sh_corefile_collect_regset.

>  shnbsd_supply_reg (struct regcache *regcache, const char *regs, int regnum)
>  {
> -  shnbsd_supply_gregset (&shnbsd_gregset, regcache, regnum,
> -			 regs, SHNBSD_SIZEOF_GREGS);
> +  sh_corefile_gregset.supply_regset (&sh_corefile_gregset, regcache, regnum,
> +				     regs, SHNBSD_SIZEOF_GREGS);
>  }

Personally, I would prefer it if you declared the two functions
sh_corefile_supply_regset and sh_corefile_collect_regset as non-static
and using them directly, rather than accessing a global constant to call
a function.  This made me believe, at first, that you were accessing
a global variable that was set while the target was determined, and thus
would not be multiarch-friendly.  I see now that this is not the case,
so the code looks correct to me now. I would just delete the code
and replace the calls form shnbsd-nat.c to the calls to the new
sh_corefile_supply/collect_regset functions.

But this is not a strong requirement.  If you prefer it the current
way, I will not complain.

-- 
Joel