[PATCH 2/3] Add FreeBSD/mips architecture.
Luis Machado
lgustavo@codesourcery.com
Fri Nov 25 22:52:00 GMT 2016
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
More information about the Binutils
mailing list