Re: [PATCH 2/3] Add FreeBSD/mips architecture.

[Luis Machado <lgustavo at codesourcery dot com>]

Code looks good. I have a few comments on other things.

On 11/23/2016 02:59 PM, John Baldwin wrote:
> @@ -0,0 +1,541 @@
> +/* Target-dependent code for FreeBSD/mips.
> +
> +   Copyright (C) 2016 Free Software Foundation, Inc.
> +
> +   This software was developed by SRI International and the University
> +   of Cambridge Computer Laboratory under DARPA/AFRL contract
> +   FA8750-10-C-0237 ("CTSRD"), as part of the DARPA CRASH research
> +   programme.
> +
> +   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/>.  */
> +
> +#include "defs.h"
> +#include "osabi.h"
> +#include "regset.h"
> +#include "trad-frame.h"
> +#include "tramp-frame.h"
> +
> +#include "fbsd-tdep.h"
> +#include "mips-tdep.h"
> +#include "mips-fbsd-tdep.h"
> +
> +#include "solib-svr4.h"
> +
> +/* Shorthand for some register numbers used below.  */
> +#define MIPS_PC_REGNUM  MIPS_EMBED_PC_REGNUM
> +#define MIPS_FP0_REGNUM MIPS_EMBED_FP0_REGNUM
> +#define MIPS_FSR_REGNUM MIPS_EMBED_FP0_REGNUM + 32
> +
> +/* Core file support. */
> +
> +/* Number of registers in `struct reg' from <machine/reg.h>.  The
> +   first 38 follow the standard MIPS layout.  The 39th holds
> +   IC_INT_REG on RM7K and RM9K processors.  The 40th is a dummy for
> +   padding.  */
> +#define MIPSFBSD_NUM_GREGS	40
> +
> +/* Number of registers in `struct fpreg' from <machine/reg.h>.  The
> +   first 32 hold floating point registers.  33 holds the FSR.  The
> +   34th is a dummy for padding.  */
> +#define MIPSFBSD_NUM_FPREGS	34

Should all the above defines be moved to the header file mips-fbsd-tdep.h?

> +
> +/* Supply a single register.  If the source register size matches the
> +   size the regcache expects, this can use regcache_raw_supply().  If
> +   they are different, this copies the source register into a buffer
> +   that can be passed to regcache_raw_supply().  */
> +
> +static void
> +mipsfbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
> +		     size_t len)

How about mips_fbsd_* for the function names? Multiple occurrences of this.

> +{
> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> +
> +  if (register_size (gdbarch, regnum) == len)
> +    {
> +      regcache_raw_supply (regcache, regnum, addr);
> +    }

No need for curly braces for single-statement blocks. Multiple 
occurrences of this.

> +/* Supply the floating-point registers stored in FPREGS to REGCACHE.
> +   Each floating-point register in FPREGS is REGSIZE bytes in
> +   length.  */
> +
> +void
> +mipsfbsd_supply_fpregs (struct regcache *regcache, int regnum,
> +			const void *fpregs, size_t regsize)
> +{
> +  const char *regs = (const char *) fpregs;

Should these const char * types be const gdb_byte * types instead? 
Multiple occurrences.

> +/* FreeBSD/mips register sets.  */
> +
> +static const struct regset mipsfbsd_gregset =
> +{
> +  NULL,
> +  mipsfbsd_supply_gregset,
> +  mipsfbsd_collect_gregset,
> +};
> +
> +static const struct regset mipsfbsd_fpregset =
> +{
> +  NULL,
> +  mipsfbsd_supply_fpregset,
> +  mipsfbsd_collect_fpregset,
> +};
> +
> +/* Iterate over core file register note sections.  */
> +
> +static void
> +mipsfbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
> +				       iterate_over_regset_sections_cb *cb,
> +				       void *cb_data,
> +				       const struct regcache *regcache)
> +{
> +  size_t regsize = mips_abi_regsize (gdbarch);
> +
> +  cb (".reg", MIPSFBSD_NUM_GREGS * regsize, &mipsfbsd_gregset,
> +      NULL, cb_data);
> +  cb (".reg2", MIPSFBSD_NUM_FPREGS * regsize, &mipsfbsd_fpregset,
> +      NULL, cb_data);
> +}
> +
> +/* Signal trampoline support.  */
> +
> +static void
> +mipsfbsd_sigframe_init (const struct tramp_frame *self,
> +			  struct frame_info *this_frame,
> +			  struct trad_frame_cache *cache,
> +			  CORE_ADDR func)
> +{
> +  struct gdbarch *gdbarch = get_frame_arch (this_frame);
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  CORE_ADDR sp, ucontext_addr, addr;
> +  int regnum;
> +  gdb_byte buf[4];
> +
> +  /* We find the appropriate instance of `ucontext_t' at a
> +     fixed offset in the signal frame.  */
> +  sp = get_frame_register_signed (this_frame,
> +				  MIPS_SP_REGNUM + gdbarch_num_regs (gdbarch));
> +  ucontext_addr = sp + 16;

We should make the fixed offset a constant with a #define. Maybe move 
those to the header file as well.

> +
> +  /* PC.  */
> +  regnum = mips_regnum (gdbarch)->pc;
> +  trad_frame_set_reg_addr (cache,
> +			   regnum + gdbarch_num_regs (gdbarch),
> +			    ucontext_addr + 20);

Same here for the PC offset.

> +
> +  /* GPRs.  */
> +  for (regnum = MIPS_AT_REGNUM, addr = ucontext_addr + 28;

Same here for all the other GPR's, and so on for the other fixed offsets.

> +       regnum <= MIPS_RA_REGNUM; regnum++, addr += 4)
> +    trad_frame_set_reg_addr (cache,
> +			     regnum + gdbarch_num_regs (gdbarch),
> +			     addr);
> +
> +  regnum = MIPS_PS_REGNUM;
> +  trad_frame_set_reg_addr (cache,
> +			   regnum + gdbarch_num_regs (gdbarch),
> +			   ucontext_addr + 152);

Here.

> +
> +  /* HI and LO.  */
> +  regnum = mips_regnum (gdbarch)->lo;
> +  trad_frame_set_reg_addr (cache,
> +			   regnum + gdbarch_num_regs (gdbarch),
> +			   ucontext_addr + 156);

Here.

> +  regnum = mips_regnum (gdbarch)->hi;
> +  trad_frame_set_reg_addr (cache,
> +			   regnum + gdbarch_num_regs (gdbarch),
> +			   ucontext_addr + 160);
> +

Here.

> +  if (target_read_memory (ucontext_addr + 164, buf, 4) == 0 &&

And here. More occurrences throughout the patch.

> +      extract_unsigned_integer (buf, 4, byte_order) != 0)
> +    {
> +      for (regnum = 0, addr = ucontext_addr + 168;
> +	   regnum < 32; regnum++, addr += 8)
> +	trad_frame_set_reg_addr (cache,
> +				 regnum + gdbarch_fp0_regnum (gdbarch),
> +				 addr);
> +      trad_frame_set_reg_addr (cache, mips_regnum (gdbarch)->fp_control_status,
> +			       addr);
> +    }
> +
> +  trad_frame_set_id (cache, frame_id_build (sp, func));
> +}
> +

> +static const struct tramp_frame mipsfbsd_sigframe =
> +{
> +  SIGTRAMP_FRAME,
> +  MIPS_INSN32_SIZE,
> +  {
> +    { 0x27a40010, -1 },		/* addiu   a0, sp, SIGF_UC */
> +    { 0x240201a1, -1 },		/* li      v0, SYS_sigreturn */
> +    { 0x0000000c, -1 },		/* syscall */
> +    { 0x0000000d, -1 },		/* break */
> +    { TRAMP_SENTINEL_INSN, -1 }
> +  },
> +  mipsfbsd_sigframe_init

These hex constants should be set with a #define. See mips-linux-tdep.c. 
More occurrences throughout the patch.


> +static void
> +mipsfbsd_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
> +{
> +  enum mips_abi abi = mips_abi (gdbarch);
> +
> +  /* Generic FreeBSD support.  */
> +  fbsd_init_abi (info, gdbarch);
> +
> +  set_gdbarch_software_single_step (gdbarch, mips_software_single_step);
> +
> +  switch (abi)
> +    {
> +      case MIPS_ABI_O32:
> +	tramp_frame_prepend_unwinder (gdbarch, &mipsfbsd_sigframe);
> +	break;
> +      case MIPS_ABI_N32:
> +	/* Float formats similar to Linux?  */

Is it similar? If so, then it should make it explicit.

> +	break;
> +      case MIPS_ABI_N64:
> +	/* Float formats similar to Linux?  */

Same here.

> +	tramp_frame_prepend_unwinder (gdbarch, &mips64fbsd_sigframe);
> +	break;
> +    }
> +
> +  /* TODO: set_gdbarch_longjmp_target  */
> +

I think we're fine without the TODO here. It can be addressed later.

Thanks,
Luis