[PATCH 1/5] Code for nds32 target

[Joseph S. Myers]

On Mon, 8 Jul 2013, Wei-cheng Wang wrote:

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

I advise reviewing this whole comment, and the Linux port in general, for 
whether it is still current.

I don't see the kernel port as of Linux 3.10, and Linux kernel policy as I 
understand it is that new architectures are expected to use the generic 
syscall interface, which only has rt_sigreturn.  So, references to the old 
non-realtime sigreturn are suspect for new architectures.  Similarly, 
references to glibc 2.1.2 are suspect for an architecture with no glibc 
port as of 2.18.  (If the Linux ABI isn't fully confirmed with the 
upstream Linux kernel community, you might want to submit the non-Linux 
parts of the GDB port and come back to the Linux parts later.)

> +int nds32_linux_sc_reg_offset[] =

Any reason this isn't const?  Likewise for various other arrays in the 
port.

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

Comments such as "As long as we use generic ucontext struct" also suggest 
you need to sort out your Linux ABI.  And I think comments with names and 
dates are best avoided (in rare cases it may be appropriate to refer in a 
comment e.g. to the list archives on sourceware.org).

> +      /* FIXME: Review me after <linux/user.h>, <asm/ptrace.h>, and SR regs
> +	 spec clear. [Harry@Mar.14.2006] */

It's not necessarily the case that every FIXME/TODO/... comment needs 
resolving for a port to go in - its fine for a port to go in that still 
has known enhancement issues.  But I'd suggest posting the list of 
enhancement issues to a public mailing list / wiki / ... and at least 
reviewing such comments in the sources and trying to resolve them.

> +/* Mapping between the general-purpose registers in `struct user'
> +   format and GDB's register array layout.
> +
> +   FIXME: fix me after <linux/user.h>, <asm/ptrace.h>,
> +   and SR regs spec clear. [Harry@Mar.14.2006]
> +
> +   Current remap layout depend on Tom's implementation in kernel header,
> +   in ptrace.h and IR spec 0.1 (Jan.20.2006 version)
> +   [Harry@Mar.16.2006]
> +
> +   Note: -1 means unable to get from ptrace syscall
> +
> +   Renumber according to arch/nds32/include/asm/ptrace.h
> +   Not sure whether NDS32_r0 or NDS32_ORIG_r0 represents real $r0.
> +   (current use NDS32_ORIG_r0)
> +   Registers not mapped: NDS32_FUCOP_CTL, NDS32_osp. (42 & 43)
> +   [Rudolph@Aug.18.2010]  */

Likewise.

> +     See: Bug 7430 - GDB can't set a hardware break point with PA
> +	  if IT/DT is on.  */

This doesn't look like GDB bug 7430.  References to "bug N" in GDB sources 
should be to the public GDB bug tracker rather than to any other bug 
tracker.

> +/* Wrapper for execute a GDB CLI command.  */
> +
> +static void
> +nds32_execute_command (char *cmd, char *arg, int from_tty)
> +{
> +  int len;
> +  char *line;
> +
> +  if (arg == NULL)
> +    arg = "";
> +  len = strlen (arg) + strlen (cmd) + 2;
> +  if (len > 1024)
> +    error (_("Command line too long."));

GNU programs should not have arbitrary limits (on lengths of commands, 
symbol names, ...).  There appear to be quite a few stack buffers etc. in 
this port that could impose such limits.

If in fact all the arguments to this function are generated internally and 
it's never possible, for any user input, for the limit to be exceeded, it 
should maybe be a gdb_assert that it isn't exceeded.  I haven't tried to 
review the various fixed-size buffers individually, but each should be 
checked to make sure no user input could ever cause the size to be 
exceeded and cause an error / truncation.

-- 
Joseph S. Myers
joseph@codesourcery.com