From: John Baldwin <jhb@freebsd.org>
To: gdb-patches@sourceware.org, Luis Machado <lgustavo@codesourcery.com>
Cc: binutils@sourceware.org
Subject: Re: [PATCH 2/3] Add FreeBSD/mips architecture.
Date: Mon, 28 Nov 2016 22:54:00 -0000 [thread overview]
Message-ID: <1881782.gRa9VDunkI@ralph.baldwin.cx> (raw)
In-Reply-To: <e562b011-1814-4e98-de50-15514fb9eb0e@codesourcery.com>
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
next prev parent reply other threads:[~2016-11-28 22:54 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-23 20:59 [PATCH 0/3] Add FreeBSD/mips targets to GDB John Baldwin
2016-11-23 20:59 ` [PATCH 3/3] Add native target for FreeBSD/mips John Baldwin
2016-11-26 4:26 ` Luis Machado
2016-11-28 22:54 ` John Baldwin
2016-11-23 20:59 ` [PATCH 2/3] Add FreeBSD/mips architecture John Baldwin
2016-11-25 22:52 ` Luis Machado
2016-11-28 22:54 ` John Baldwin [this message]
2016-11-23 20:59 ` [PATCH 1/3] Use the ELF class to determine the word size for FreeBSD core notes John Baldwin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1881782.gRa9VDunkI@ralph.baldwin.cx \
--to=jhb@freebsd.org \
--cc=binutils@sourceware.org \
--cc=gdb-patches@sourceware.org \
--cc=lgustavo@codesourcery.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).