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

[John Baldwin <jhb at freebsd dot org>]

On Friday, November 25, 2016 04:52:31 PM Luis Machado wrote:
> On 11/23/2016 02:59 PM, John Baldwin wrote:
> > +/* 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?

They are only used in this file by virtue of the callbacks being exported for
use by the native target.  I wasn't inclined to export more to the header
than is necessary.  (This matches the approach used in mips-nbsd-tdep.* FWIW.)

> > +
> > +/* 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.

The reason it uses the current scheme is to match other *BSD targets (both
mipsnbsd_* and mips64obsd_*, but also other platforms such as x86bsd_* and
amd64bsd_*, etc.)  On the other hand, Simon just renamed all the BSD target
files to add a - between the architecture and OS, so that pattern might be
an old one.  I'm fine using mips_* for new code if that is generally desired.

> > +{
> > +  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.

Ok.  (Too many different style guides to juggle in one's head) 

> > +/* 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.

Hmm, perhaps?  The NetBSD/mips target I used as a template uses 'char',
but other places use gdb_byte (e.g. amd64_collect_*).  I'll go ahead
and switch it.

> > +/* 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.

FWIW, mips64-obsd just used magic numbers (which is what I used as my
template for the signal frame bits), but I can add constants for the
layout of ucontext_t, etc.  I'm inclined to keep them in the C file
as they won't be used outside of it.

> > +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.

Will fix.  As above, this was copied from the mips64-obsd target.

> > +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.

I don't know yet.  I have another pending patch to do so, but I think
FreeBSD/mips is currently using long double == double on n32 and n64
rather than 128-bit long doubles (as Linux, NetBSD, and OpenBSD all
seem to do).

> > +	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.

I left it as a reminder for myself, but I can axe it.

-- 
John Baldwin