This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 1/5] Code for nds32 target


On 07/08/2013 05:27 PM, Wei-cheng Wang wrote:

diff --git a/gdb/features/feature_to_c.sh b/gdb/features/feature_to_c.sh
old mode 100644
new mode 100755

Unnecessary change.

diff --git a/gdb/features/nds32-core.xml b/gdb/features/nds32-core.xml
new file mode 100644
index 0000000..8de8059
--- /dev/null
+++ b/gdb/features/nds32-core.xml
@@ -0,0 +1,56 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2007, 2008, 2009, 2010 Free Software Foundation, Inc.
+
+     Copying and distribution of this file, with or without modification,
+     are permitted in any medium without royalty provided the copyright
+     notice and this notice are preserved.  -->
+
+<!DOCTYPE feature SYSTEM "gdb-target.dtd">
+<feature name="org.gnu.gdb.nds32.core">
+  <reg name="r0" bitsize="32" regnum="0"/>
+  <reg name="r1" bitsize="32"/>
+  <reg name="r2" bitsize="32"/>
+  <reg name="r3" bitsize="32"/>
+  <reg name="r4" bitsize="32"/>
+  <reg name="r5" bitsize="32"/>
+  <reg name="r6" bitsize="32"/>
+  <reg name="r7" bitsize="32"/>
+  <reg name="r8" bitsize="32"/>
+  <reg name="r9" bitsize="32"/>
+  <reg name="r10" bitsize="32"/>
+  <reg name="r11" bitsize="32"/>
+  <reg name="r12" bitsize="32"/>
+  <reg name="r13" bitsize="32"/>
+  <reg name="r14" bitsize="32"/>
+  <reg name="r15" bitsize="32" regnum="15"/>

Do we really need "regnum="15"" here?

diff --git a/gdb/nds32-linux-nat.c b/gdb/nds32-linux-nat.c
new file mode 100644
index 0000000..2f87822
--- /dev/null
+++ b/gdb/nds32-linux-nat.c
@@ -0,0 +1,299 @@
+/* Common target dependent code for GDB on nds32 systems.
+
+   Copyright (C) 2006-2013 Free Software Foundation, Inc.
+   Contributed by Andes Technology Corporation.
+
+   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 "inferior.h"
+#include "gdbcore.h"
+#include "gdb_string.h"
+#include "regcache.h"
+#include "target.h"
+#include "linux-nat.h"
+#include "target-descriptions.h"
+#include "auxv.h"
+
+#include <elf/common.h>
+#include <sys/user.h>
+#include <sys/ptrace.h>
+#include <sys/utsname.h>
+#include <sys/procfs.h>
+
+/* Prototypes for supply_gregset etc. */
+#include "gregset.h"
+
+/* Defines ps_err_e, struct ps_prochandle.  */
+#include "gdb_proc_service.h"
+
+#include "nds32-tdep.h"
+#include "nds32-linux-tdep.h"
+#include "features/nds32-linux.c"

Missing include these four files?

features/nds32-freg0-linux.c
features/nds32-freg1-linux.c
features/nds32-freg2-linux.c
features/nds32-freg3-linux.c

+
+/* Fetch the thread-local storage pointer for libthread_db.  */
+
+#if 0
+/* TODO: TLS is not supported, yet.  */
+
+ps_err_e
+ps_get_thread_area (const struct ps_prochandle *ph,
+		    lwpid_t lwpid, int idx, void **base)
+{
+}
+#endif
+

Remove #if0 ... #endif.

diff --git a/gdb/nds32-linux-tdep.c b/gdb/nds32-linux-tdep.c
new file mode 100644
index 0000000..79d51cd
--- /dev/null
+++ b/gdb/nds32-linux-tdep.c
@@ -0,0 +1,320 @@
+/* Common target dependent code for GDB on nds32 systems.

The header comment of all nds32-*.c files are "Common target dependent code for GDB on nds32 systems.", which makes no sense to me. Please give the meaningful comment to each of them.

+
+   Copyright (C) 2006-2013 Free Software Foundation, Inc.
+   Contributed by Andes Technology Corporation.
+
+   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 "gdbcore.h"
+#include "frame.h"
+#include "value.h"
+#include "regcache.h"
+#include "inferior.h"
+#include "osabi.h"
+#include "reggroups.h"
+#include "regset.h"
+#include "target-descriptions.h"
+
+#include "gdb_string.h"
+
+#include "glibc-tdep.h"
+#include "solib-svr4.h"
+
+#include "trad-frame.h"
+#include "frame-unwind.h"
+
+#include "nds32-tdep.h"
+#include "nds32-linux-tdep.h"
+
+extern struct nds32_gdb_config nds32_config;
+

nds32_config is not used in this file.  Remove this declaration.

+void _initialize_nds32_linux_tdep (void);
+
+/* Recognizing signal handler frames.  */
+
+/* GNU/Linux has two flavors of signals.  Normal signal handlers, and
+   "realtime" (RT) signals.  The RT signals can provide additional
+   information to the signal handler if the SA_SIGINFO flag is set
+   when establishing a signal handler using `sigaction'.  It is not
+   unlikely that future versions of GNU/Linux will support SA_SIGINFO
+   for normal signals too.  */
+
+/* When the NDS32 Linux kernel calls a signal handler and the
+   SA_RESTORER flag isn't set, the return address points to a bit of
+   code on the stack.  This function returns whether the PC appears to
+   be within this bit of code.
+
+   The instructions for normal and realtime signals are
+       syscall   #__NR_sigreturn ( 0x26 0x01 0xDC 0x0B)
+       or
+       syscall   #__NR_rt_sigreturn ( 0x26 0x02 0xB4 0x0B)
+
+   Checking for the code sequence should be somewhat reliable, because
+   the effect is to call the system call sigreturn.  This is unlikely
+   to occur anywhere other than in a signal trampoline.
+
+   It kind of sucks that we have to read memory from the process in
+   order to identify a signal trampoline, but there doesn't seem to be
+   any other way.  Therefore we only do the memory reads if no
+   function name could be identified, which should be the case since
+   the code is on the stack.
+
+   Detection of signal trampolines for handlers that set the
+   SA_RESTORER flag is in general not possible.  Unfortunately this is
+   what the GNU C Library has been doing for quite some time now.
+   However, as of version 2.1.2, the GNU C Library uses signal
+   trampolines (named __restore and __restore_rt) that are identical
+   to the ones used by the kernel.  Therefore, these trampolines are
+   supported too.  */
+
+/* syscall #0x5077 */
+static const unsigned char NDS32_SIGRETURN_INSN[] =
+{
+  0x64, 0x0a, 0x0e, 0xeb
+};
+
+/* syscall #0x50ad */
+static const unsigned char NDS32_RT_SIGRETURN_INSN[] =
+{
+  0x64, 0x0a, 0x15, 0xab
+};
+
+int nds32_linux_sc_reg_offset[] =
+{
+  /* r0 - r9 */
+  12, 16, 20, 24, 28, 32, 36, 40, 44, 48,
+  /* r10 - r19 */
+  52, 56, 60, 64, 68, 72, 76, 80, 84, 88,
+  /* r20 - r25, 26, 27 */
+  92, 96, 100, 104, 108, 112, -1, -1,
+  /* fp, gp, lr, sp */
+  116, 120, 124, 128,
+  /* pc, d0lo, d0hi, d1lo, d1hi */
+  148, 140, 144, 132, 136
+};
+
+struct nds32_frame_cache
+{
+  CORE_ADDR base, pc;
+  struct trad_frame_saved_reg *saved_regs;
+};
+
+/* Find start of signal frame.  */
+
+static CORE_ADDR
+nds32_linux_sigtramp_start (struct frame_info *this_frame)
+{
+  const int SIGLEN = sizeof (NDS32_SIGRETURN_INSN);
+  CORE_ADDR pc = get_frame_pc (this_frame);
+  gdb_byte buf[SIGLEN];
+
+  if (!safe_frame_unwind_memory (this_frame, pc, buf, SIGLEN))
+    return 0;
+
+  if (memcmp (buf, NDS32_SIGRETURN_INSN, SIGLEN) != 0)
+    return 0;
+
+  return pc;
+}
+
+/* Find start of rt_signal frame.  */
+
+static CORE_ADDR
+nds32_linux_rt_sigtramp_start (struct frame_info *this_frame)
+{
+  const int SIGLEN = sizeof (NDS32_RT_SIGRETURN_INSN);
+  CORE_ADDR pc = get_frame_pc (this_frame);
+  gdb_byte buf[SIGLEN];
+
+  if (!safe_frame_unwind_memory (this_frame, pc, buf, SIGLEN))
+    return 0;
+
+  if (memcmp (buf, NDS32_RT_SIGRETURN_INSN, SIGLEN) != 0)
+    return 0;
+
+  return pc;
+}
+
+/* Return whether the frame preceding NEXT_FRAME corresponds to a
+   GNU/Linux sigtramp routine.  */
+
+static int
+nds32_linux_sigtramp_p (struct frame_info *this_frame)
+{
+  CORE_ADDR pc = get_frame_pc (this_frame);
+  const char *name;
+
+  find_pc_partial_function (pc, &name, NULL, NULL);
+
+  /* If we have NAME, we can optimize the search.  The trampolines are
+     named __restore and __restore_rt.  However, they aren't dynamically
+     exported from the shared C library, so the trampoline may appear to
+     be part of the preceding function.  This should always be sigaction,
+     __sigaction, or __libc_sigaction (all aliases to the same function).  */
+  if (name == NULL)
+    return (nds32_linux_sigtramp_start (this_frame) != 0
+	    || nds32_linux_rt_sigtramp_start (this_frame) != 0);
+
+  return (strcmp ("__default_sa_restorer", name) == 0
+	  || strcmp ("__default_rt_sa_restorer", name) == 0);
+}
+
+/* Offset to struct sigcontext in ucontext, from <asm/ucontext.h>.  */
+#define NDS32_LINUX_UCONTEXT_SIGCONTEXT_OFFSET 0x18
+
+/* Assuming NEXT_FRAME is a frame following a GNU/Linux sigtramp
+   routine, return the address of the associated sigcontext structure.  */
+
+static CORE_ADDR
+nds32_linux_sigcontext_addr (struct frame_info *this_frame)
+{
+  CORE_ADDR pc;
+  CORE_ADDR sp;
+  gdb_byte buf[4];
+
+  sp = get_frame_sp (this_frame);
+
+  /* sigcontext is at sp for sigtramp */
+  pc = nds32_linux_sigtramp_start (this_frame);
+  if (pc)
+    return sp;
+
+  pc = nds32_linux_rt_sigtramp_start (this_frame);
+  if (pc)
+    {
+      CORE_ADDR ucontext_addr;
+      int r2;
+
+      /* Cole, Dec. 31th, 2010
+	 sigcontext is stored in frame->uc.uc_mcontext, Therefore,
+	 there are two ways to get sigcontext.
+	 The first way, direct access it in the stack.In this way,
+	 we needs more knowledge of rt_sigtramp
+	 The second way, &us is passed as parameter 3 of handler,
+	 that would be R2 in NDS32 ABI.As long as we use generic
+	 ucontext struct, I think it's easier to get sigcontext.  */
+
+      r2 = get_frame_register_unsigned (this_frame, NDS32_R0_REGNUM + 2);
+      sp = r2;
+      /* This value is dependent on kernel.  */
+      sp += NDS32_LINUX_UCONTEXT_SIGCONTEXT_OFFSET;
+      return sp;
+    }
+
+  error (_("Couldn't recognize signal trampoline."));
+  return 0;
+}

For all the signal trampoline frame stuff, why don't you use 'tramp_frame', which is simpler than your current approach. Please grep tramp_frame_prepend_unwinder to see how it is used in other *-tdep.c files.

+
+static void
+nds32_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
+{
+  const struct target_desc *tdesc = info.target_desc;
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+  struct tdesc_arch_data *tdesc_data = (void *) info.tdep_info;
+  const struct tdesc_feature *feature;
+
+  tdep->sigtramp_p = nds32_linux_sigtramp_p;
+  tdep->sigcontext_addr = nds32_linux_sigcontext_addr;
+  tdep->sc_pc_offset = 37 * 4;	/* sc.fault_address */
+  tdep->sc_sp_offset = 32 * 4;	/* sc.sp */
+  tdep->sc_lp_offset = 31 * 4;	/* sc.lp */
+  tdep->sc_fp_offset = 29 * 4;	/* sc.fp */
+
+  tdep->sc_reg_offset = nds32_linux_sc_reg_offset;
+  tdep->sc_num_regs = ARRAY_SIZE (nds32_linux_sc_reg_offset);
+
+  /* GNU/Linux uses SVR4-style shared libraries.  */
+  set_solib_svr4_fetch_link_map_offsets (gdbarch,
+					 svr4_ilp32_fetch_link_map_offsets);
+
+  /* Core file support.  */
+  /* FIXME: Cole, Dec 31th, 2010
+     It seems this doesn't work? */

Does it work? :)

diff --git a/gdb/nds32-remote.c b/gdb/nds32-remote.c
new file mode 100644
index 0000000..c088f0d
--- /dev/null
+++ b/gdb/nds32-remote.c

Since there are no comments on what nds32-remote.c is for, I assume that it is used to talk with remote target, like jtag ice. IIUC, you invent some new rsp packets to your own targets, but it is not recommended to add such thing into GDB nowadays. I can't find the mail archive about this decision, and other people can give a definitive answer here.

Probably, you can move nds32-remote.c to a separate patch, so that it won't block the other patches. On the other hand, it is not a big deal to maintain nds32-remote.c by yourself.

diff --git a/gdb/nds32-tdep.c b/gdb/nds32-tdep.c
new file mode 100644
index 0000000..f861807
--- /dev/null
+++ b/gdb/nds32-tdep.c
@@ -0,0 +1,2802 @@
+/* Common target dependent code for GDB on nds32 systems.
+
+   Copyright (C) 2006-2013 Free Software Foundation, Inc.
+   Contributed by Andes Technology Corporation.
+
+   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 <assert.h>

Remove this include?

+#include <stdint.h>
+
+#include "defs.h"

Include "defs.h" first.

+#include "frame.h"
+#include "frame-unwind.h"
+#include "frame-base.h"
+#include "symtab.h"
+#include "gdbtypes.h"
+#include "gdbcmd.h"
+#include "gdbcore.h"
+#include "gdb_string.h"
+#include "value.h"
+#include "reggroups.h"
+#include "inferior.h"
+#include "symfile.h"
+#include "objfiles.h"
+#include "osabi.h"
+#include "language.h"
+#include "arch-utils.h"
+#include "regcache.h"
+#include "trad-frame.h"
+#include "dis-asm.h"
+#include "gdb_assert.h"
+#include "user-regs.h"
+#include "elf-bfd.h"
+#include "dwarf2-frame.h"
+#include "ui-file.h"
+#include "remote.h"
+#include "target-descriptions.h"
+#include "sim-regno.h"
+#include "gdb/sim-nds32.h"
+
+#include "nds32-tdep.h"
+#include "nds32-utils.h"
+#include "nds32-remote.h"
+#include "elf/nds32.h"
+#include "opcode/nds32.h"
+
+/* Simple macro for chop LSB immediate bits from an instruction.  */
+#define CHOP_BITS(insn, n)	(insn & ~__MASK (n))
+
+extern void _initialize_nds32_tdep (void);
+
+struct nds32_gdb_config nds32_config;

Make it static.

+
+/* Register alias for user_reg_map_name_to_regnum ().  */
+struct nds32_register_alias nds32_register_aliases[] =

Make it static too.

+
+/* Value of a register alias.  BATON is register name of the alias,
+   because system registers do not have fixed register number.
+   We must look-up them when access.  */
+
+static struct value *
+nds32_value_of_reg (struct frame_info *frame, const void *baton)
+{
+  struct gdbarch *gdbarch = get_frame_arch (frame);
+  int regnum;
+
+  regnum = user_reg_map_name_to_regnum (gdbarch, (const char *) baton, -1);
+
+  return value_of_register (regnum, frame);
+}
+
+/* gdbarch_frame_align () */

/* This is the implementation of gdbarch method frame_align.  */

+
+static CORE_ADDR
+nds32_frame_align (struct gdbarch *gdbarch, CORE_ADDR sp)
+{
+  /* 8-byte aligned.  */
+  return sp & ~(8 - 1);

return align_down (sp, 8);

+}
+
+/* Implement the gdbarch_breakpoint_from_pc method.  */
+
+static const gdb_byte *
+nds32_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr,
+			  int *lenptr)
+{
+  static const gdb_byte NDS32_BREAK16[] = { 0xEA, 0x00 };

Do we need to worry about big-endian and little endian here?

+  const unsigned char *bp;
+
+  gdb_assert (pcptr);

     gdb_assert (pcptr != NULL);

See 16.1.4 C Usage in http://www.sourceware.org/gdb/onlinedocs/gdbint.html

+  gdb_assert (lenptr);
+

Likewise.

+  if ((*pcptr) & 1)
+    error (_("Bad address %p for inserting breakpoint.\n"
+	     "Address must be at least 2-byte aligned."), (void *) *pcptr);

Do you know when the last bit of address is 1? I am wondering whether this checking is necessary or it is caused by the bug somewhere else.

+
+  /* Always insert 16-bit break instruction.  */
+  *lenptr = 2;
+  return NDS32_BREAK16;
+}
+

+
+/* Create types for registers and insert them to type table by name.  */
+
+static void
+nds32_alloc_types (struct gdbarch *gdbarch)
+{
+  const struct builtin_type *bt = builtin_type (gdbarch);
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+  struct type *type, *stype1, *stype2;
+
+  tdep->type_tab = nds32_alloc_type_tab (24);
+
+  /* fucpr */
+  type = arch_flags_type (gdbarch, "builtin_type_nds32_fucpr", 4);
+  append_flags_type_flag (type, 0, "CP0EN");
+  append_flags_type_flag (type, 1, "CP1EN");
+  append_flags_type_flag (type, 2, "CP2EN");
+  append_flags_type_flag (type, 3, "CP3EN");
+  append_flags_type_flag (type, 31, "AUEN");
+  nds32_type_insert (tdep->type_tab, "fucpr", type);

Can you define 'fucpr' in the target description? like

  <flags id="fucpr" size="4">
     <field name="CP0EN" start="0" end="0"/>
     .....
  </flags>

which is simpler, IMO.

+
+/* gdbarch_register_type ()

/* This is the implementation of gdbarch method register_type.  */

+
+   Return the GDB type object for the "standard" data type
+   of data in register N.
+   It get pretty messy here. I need enum-types and bit-fields
+   for better representation. But they cannot be done by
+   tdesc-xml.  */

target description works for enum and bitfields. For example, in features/i386/32bit-core.xml,

  <flags id="i386_eflags" size="4">
    <field name="CF" start="0" end="0"/>
    <field name="" start="1" end="1"/>
    <field name="PF" start="2" end="2"/>
    <field name="AF" start="4" end="4"/>
    <field name="ZF" start="6" end="6"/>
    <field name="SF" start="7" end="7"/>
    <field name="TF" start="8" end="8"/>
    <field name="IF" start="9" end="9"/>
    <field name="DF" start="10" end="10"/>
    <field name="OF" start="11" end="11"/>
    <field name="NT" start="14" end="14"/>
    <field name="RF" start="16" end="16"/>
    <field name="VM" start="17" end="17"/>
    <field name="AC" start="18" end="18"/>
    <field name="VIF" start="19" end="19"/>
    <field name="VIP" start="20" end="20"/>
    <field name="ID" start="21" end="21"/>
  </flags>

I have to stop reviewing here as I've run out of time of patch review today. I may have a look at the rest of the patch tomorrow.

As Joseph mentioned, there are a lot of FIXME/TODO/review, probably you have to get rid of them first.

I noticed that some functions in nds32-utils.c are not used at all. Please remove them. Some functions are only used in nds32-tdep.c, so probably we can move them to nds32-tdep.c, and make them static.

--
Yao (éå)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]