From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4995 invoked by alias); 8 Jul 2013 23:58:42 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 4982 invoked by uid 89); 8 Jul 2013 23:58:40 -0000 X-Spam-SWARE-Status: No, score=-3.4 required=5.0 tests=AWL,BAYES_50,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL autolearn=ham version=3.3.1 Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Mon, 08 Jul 2013 23:58:38 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1UwLKF-000177-VC from joseph_myers@mentor.com ; Mon, 08 Jul 2013 16:58:35 -0700 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Mon, 8 Jul 2013 16:58:36 -0700 Received: from digraph.polyomino.org.uk (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.2.247.3; Tue, 9 Jul 2013 00:58:33 +0100 Received: from jsm28 (helo=localhost) by digraph.polyomino.org.uk with local-esmtp (Exim 4.76) (envelope-from ) id 1UwLKC-0003Uh-Ly; Mon, 08 Jul 2013 23:58:32 +0000 Date: Mon, 08 Jul 2013 23:58:00 -0000 From: "Joseph S. Myers" To: Wei-cheng Wang CC: Subject: Re: [PATCH 1/5] Code for nds32 target In-Reply-To: Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" X-SW-Source: 2013-07/txt/msg00239.txt.bz2 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 , , 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 , , > + 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