Re: [RFC] QNX Neutrino/i386 support

Mark Kettenis
2003-03-09T00:14:25Z

Kris,

Here are my comments about the i386-nto-tdep.c file. Some general
stuff first (which probably applies to the other files as well):

* Please review the GNU and GDB coding standards, there are quite a
few violations in the QNX files, most notably:

- Always make comments full sentences. Start with a capital, end
with a full stop, and always use two spaces after it, even if its
the last sentence of the comment. Format the comments according
to the GNU coding standards.

- Use ISO function declarations.

- Don't use the register keyword.

* Your files should include the blurb about the GPL. Look at the
files that are already part of GDB.

My file-specific comments are given below intertwined with bits from
the file itself. Search for >>> to get at my comments:

/* i386-nto-tdep.c - i386 specific functionality for QNX Neutrino */

/* Copyright 2003 Free Software Foundation */

/* This code was donated by QNX Software Systems Ltd. */

>>> Other files in GDB say "Contributed by XYZ, Inc.", so please
>>> consider using the same style.

#include "defs.h"
#include "gdb_string.h"
#include <fcntl.h>
#include "frame.h"
#include "inferior.h"
#include "bfd.h"
#include "symfile.h"
#include "target.h"
#include "gdb_wait.h"
#include "gdbcmd.h"
#include "objfiles.h"
#include "gdb-stabs.h"
#include "gdbthread.h"
#include "regcache.h"
#include "solib-svr4.h"
#include "dcache.h"
#include "i386-tdep.h"
#include "nto-tdep.h"
#include "osabi.h"

>>> That's a while lot of includes. I really doubt whether you need
>>> them all. Please trim.

#ifdef USG
#include <sys/types.h>
#endif

>>> We shouldn't introduce any new uses of the USG define.

#include <signal.h>
#include "serial.h"

#include "nto-share/debug.h"

/* Prototypes for i387_supply_fsave etc. */
#include "i387-tdep.h"

#ifndef FPC_REGNUM
#define FPC_REGNUM (FP0_REGNUM + 8)
#endif

>>> FPC is already defined by i386-tdep.h.

#define NUM_GPREGS 13
static int gdb_to_nto[NUM_GPREGS] =
{ 7, 6, 5, 4, 11, 2, 1, 0, 8, 10, 9, 12, -1 };

#define GDB_TO_OS(x) ((x >= 0 && x < NUM_GPREGS) ? gdb_to_nto[x] : -1)

#ifndef X86_CPU_FXSR
#define X86_CPU_FXSR (1L << 12)
#endif

extern unsigned nto_cpuinfo_flags;
extern int nto_cpuinfo_valid;
extern int nto_ostype, nto_cputype;

>>> Are these global variables really needed? If they are, they
>>> probably belong in a header file of some sort, and they would
>>> deserve a comment.

/*
Given a register return an id that represents the Neutrino regset it came from
if reg == -1 update all regsets
$MJC
*/

enum QNX_REGS
{
QNX_REGS_GP = 0,
QNX_REGS_FP = 1,
QNX_REGS_END = 2
};

unsigned
nto_get_regset_id (int regno)
{
unsigned regset;
if (regno == -1)
{ /* return size so we can enumerate */
regset = 2;
}
else if (regno < FP0_REGNUM)
{
regset = 0;
}
else if (regno < FPC_REGNUM)
{
regset = 1;
}
else
{
regset = -1; /* error */
}
return regset;
}

>>> Personally, I would leave out the braces here.

/* Tell GDB about the regset contained in the 'data' buffer */
static void
gp_regset_fetch (char *data)
{
unsigned first_regno;
for (first_regno = 0; first_regno < FP0_REGNUM; first_regno++)
{
int regno = GDB_TO_OS (first_regno);
int const minusone = -1;

if (data == NULL || regno == -1)
{
supply_register (first_regno, (char *) &minusone);

>>> GDB's convention is to fill registers with zero instead of -1. At
>>> least that's what all the other i386 targets do.

}
else
{
supply_register (first_regno, &data[regno * 4]);
}
}
}

/* get 8087 data */
static void
fp_regset_fetch (char *data)
{
if (nto_cpuinfo_valid && nto_cpuinfo_flags | X86_CPU_FXSR)
i387_supply_fxsave (data);
else
i387_supply_fsave (data);
}

void
nto_cpu_regset_fetch (int endian, int regset, void *data)
{
endian = endian; /* x86, don't care about endian */

switch (regset)
{
case QNX_REGS_GP: /* QNX has different ordering of GP regs GDB */
gp_regset_fetch ((char *) data);
break;
case QNX_REGS_FP:
fp_regset_fetch ((char *) data);
break;
}
}

/* get regset characteristics */
unsigned
nto_get_regset_area (unsigned regset, char *subcmd)
{
unsigned length = 0;
switch (regset)
{
case QNX_REGS_GP:
*subcmd = NTO_REG_GENERAL;
length = NUM_GPREGS * sizeof (unsigned);
break;
case QNX_REGS_FP:
*subcmd = NTO_REG_FLOAT;
/* FIXME: should we calculate based on fxsave/fsave? */
length = 512;
break;
default:
length = 0;
}
return length;
}

/*-
Given the first and last register number, figure out the size/len
of the Neutrino register save area to ask for/tell about. Also set
the register set that's being dealt with in *subcmd. Watch out for
the range crossing a register set boundry.
*/

unsigned
nto_cpu_register_area (first_regno, last_regno, subcmd, off, len)
unsigned first_regno;
unsigned last_regno;
unsigned char *subcmd;
unsigned *off;
unsigned *len;
{
int regno = -1;

if (first_regno < FP0_REGNUM)
{
if (last_regno >= FP0_REGNUM)
last_regno = FP0_REGNUM - 1;
*subcmd = NTO_REG_GENERAL;
regno = GDB_TO_OS (first_regno);
*off = regno * sizeof (unsigned);
if (regno == -1)
*len = 0;
else
*len = (last_regno - first_regno + 1) * sizeof (unsigned);
}
else if (first_regno == FP_REGNUM)
{
/* Frame Pointer Psuedo-register */
*off = SP_REGNUM * sizeof (unsigned);
*len = sizeof (unsigned);
return FP_REGNUM;
}
else if (first_regno >= FP0_REGNUM && first_regno < FPC_REGNUM)
{
unsigned off_adjust, regsize;

if (nto_cpuinfo_valid && nto_cpuinfo_flags | X86_CPU_FXSR)
{
off_adjust = 32;
regsize = 16;
}
else
{
off_adjust = 28;
regsize = 10;
}

if (last_regno >= FPC_REGNUM)
last_regno = FPC_REGNUM - 1;
*subcmd = NTO_REG_FLOAT;
*off = (first_regno - FP0_REGNUM) * regsize + off_adjust;
*len = (last_regno - first_regno + 1) * 10;
/* Why 10? GDB only stores 10 bytes per FP register so if we're
* sending a register back to the target, we only want pdebug to write
* 10 bytes so as not to clobber the reserved 6 bytes in the fxsave
* structure. The astute reader will note that this will fail if we
* try to send a range of fpregs rather than 1 at a time but, as far
* as I know, there is no way to send more than one fpreg at a time
* anyway. If this turns out to be wrong, we may need to put more code
* in pdebug to deal with this - perhaps by masking off part of the
* register when it writes it in.*/
}
else
{
*len = 0;
return last_regno;
}
return last_regno;
}

/* Tell GDB about the registers contained in the 'data' buffer */

void
nto_cpu_register_fetch (endian, first_regno, last_regno, data)
int endian;
unsigned first_regno;
unsigned last_regno;
void *data;
{
fprintf_unfiltered (gdb_stderr, "warning: nto_cpu_register_fetch called \
in remote-nto-i386.c. This shouldn't happen\n");
/* If remote-nto-i386.c calls nto_supply_register in remote-nto.c,
* then remote-nto.c calls nto_cpu_register_fetch. Since this never
* happens, this function is left unimplemented. */
}

>>> This comment references remote-i386-nto.c. There is no such file
>>> in your patch.

/* Build the Neutrino register set info into the 'data' buffer */

int
nto_cpu_register_store (endian, first_regno, last_regno, data)
int endian;
unsigned first_regno;
unsigned last_regno;
void *data;
{
/* Mostly (always?) you're only storing one at a time */
if (first_regno == last_regno)
{
regcache_collect (first_regno, data);
return 1;
}
/* Floating point is the same for gdb and target */
if (first_regno >= FP0_REGNUM)
{
for (; first_regno <= last_regno; first_regno++)
{
regcache_collect (first_regno, data);
(char *) data += REGISTER_RAW_SIZE (first_regno);
}
return 1;
}
/* GP registers are mapped differently for NTO than GDB */
for (; first_regno <= last_regno; first_regno++)
{
int regnum = GDB_TO_OS (first_regno);
if (regnum == -1)
continue;
regcache_collect (first_regno,
(char *) data + sizeof (unsigned) * regnum);
}
return 1;
}

/* nto_supply_* are used by nto_procfs.c and nto_core-regset.c */
void
nto_supply_gregset (gregsetp)
nto_gregset_t *gregsetp;
{
register int regi, i = 0;
register unsigned *regp = (unsigned *) gregsetp;

for (regi = 0; regi < (NUM_REGS - FP0_REGNUM); regi++)
{
i = GDB_TO_OS (regi);
supply_register (regi, (char *) &regp[i]);
}
}

void
nto_supply_fpregset (fpregsetp)
nto_fpregset_t *fpregsetp;
{
if (nto_cpuinfo_valid && nto_cpuinfo_flags | X86_CPU_FXSR)
i387_supply_fxsave ((char *) fpregsetp);
else
i387_supply_fsave ((char *) fpregsetp);
}

/* Fetch (and possibly build) an appropriate link_map_offsets
structure for native x86 targets using the struct offsets
defined in link.h (but without actual reference to that file).

This makes it possible to access x86 shared libraries from a GDB
that was not built on an x86 host (for cross debugging). */

static struct link_map_offsets *
i386_nto_svr4_fetch_link_map_offsets (void)
{
static struct link_map_offsets lmo;
static struct link_map_offsets *lmp = NULL;

if (lmp == NULL)
{
lmp = &lmo;

lmo.r_debug_size = 8; /* The actual size is 20 bytes, but
this is all we need. */
lmo.r_map_offset = 4;
lmo.r_map_size = 4;

lmo.link_map_size = 20; /* The actual size is 552 bytes, but
this is all we need. */
lmo.l_addr_offset = 0;
lmo.l_addr_size = 4;

lmo.l_name_offset = 4;
lmo.l_name_size = 4;

lmo.l_next_offset = 12;
lmo.l_next_size = 4;

lmo.l_prev_offset = 16;
lmo.l_prev_size = 4;
}

return lmp;
}

struct link_map_offsets *
nto_fetch_link_map_offsets (void)
{
return i386_nto_svr4_fetch_link_map_offsets ();
}

static int
i386_nto_pc_in_sigtramp (CORE_ADDR pc, char *name)
{
return name && strcmp ("__signalstub", name) == 0;
}

static CORE_ADDR
i386_nto_sigcontext_addr (struct frame_info *frame)
{
int sigcontext_offset = 136;

if (get_next_frame (frame))
return get_frame_base (get_next_frame (frame)) + sigcontext_offset;

return read_register (SP_REGNUM) + sigcontext_offset;
}

static void
i386_nto_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
{
struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);

/* NTO uses ELF. */
i386_elf_init_abi (info, gdbarch);

/* neutrino rewinds to look more normal */
set_gdbarch_decr_pc_after_break (gdbarch, 0);

/* NTO has shared libraries. */
set_gdbarch_in_solib_call_trampoline (gdbarch, in_plt_section);
set_gdbarch_skip_trampoline_code (gdbarch, find_solib_trampoline_target);

set_gdbarch_pc_in_sigtramp (gdbarch, i386_nto_pc_in_sigtramp);
tdep->sigcontext_addr = i386_nto_sigcontext_addr;
tdep->sc_pc_offset = 56;
tdep->sc_sp_offset = 68;

/* setjmp()'s return PC saved in EDX (5) */
tdep->jb_pc_offset = 5 * sizeof (int);

>>> You cannot use sizeof(int) here, since that might very well differ
>>> from 4 on other systems.

set_solib_svr4_fetch_link_map_offsets (gdbarch,
i386_nto_svr4_fetch_link_map_offsets);

/* Our loader handles solib relocations slightly differently than svr4 */
TARGET_SO_RELOCATE_SECTION_ADDRESSES = nto_relocate_section_addresses;

/* Supply a nice function to find our solibs */
TARGET_SO_FIND_AND_OPEN_SOLIB = nto_find_and_open_solib;

/* After a watchpoint trap, the PC points to the instruction after
the one that caused the trap. Therefore we don't need to step over it.
But we do need to reset the status register to avoid another trap. */
HAVE_CONTINUABLE_WATCHPOINT = 1;

>>> This HAVE_CONTINUABLE_WATCHPOINT stuff has been moved to the
>>> target vector, so that's where you should provide the appropriate
>>> definition.

}

void
_initialize_i386_nto_tdep (void)
{
gdbarch_register_osabi (bfd_arch_i386, 0, GDB_OSABI_QNXNTO,
i386_nto_init_abi);

/* read our extra gdbinit file */
nto_source_extra_gdbinit (".ntox86-gdbinit");

>>> This extra-gdbinit-stuff doesn't belong here. We have had a
>>> discussion about these target-dependent init files recently, and I
>>> believe the outcome was that if they were wanted, some general
>>> mechanism should be added rather than these kind of ad-hoc hacks.

}