From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28488 invoked by alias); 7 Dec 2012 02:42:34 -0000 Received: (qmail 28480 invoked by uid 22791); 7 Dec 2012 02:42:33 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_NO,TW_EG X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 07 Dec 2012 02:42:00 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 535B32E192; Thu, 6 Dec 2012 21:41:59 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id Hy2E1LPrdLWs; Thu, 6 Dec 2012 21:41:59 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id B5F962E180; Thu, 6 Dec 2012 21:41:58 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 1E1F3C0E21; Fri, 7 Dec 2012 06:41:45 +0400 (RET) Date: Fri, 07 Dec 2012 02:42:00 -0000 From: Joel Brobecker To: Jiong Wang Cc: Yao Qi , gdb-patches@sourceware.org, Walter Lee , palves@redhat.com Subject: Re: [PATCH/tilegx] tilegx bug fixes & improvements Message-ID: <20121207024144.GB31477@adacore.com> References: <50C04269.90304@tilera.com> <50C067CC.2050303@codesourcery.com> <50C0B790.9070509@tilera.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50C0B790.9070509@tilera.com> User-Agent: Mutt/1.5.21 (2010-09-15) 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 X-SW-Source: 2012-12/txt/msg00122.txt.bz2 > >alloca is unsafe, and we prefer to use xmalloc/cleanup+xfree. > > fixed. Just to be more precise, we avoid the use of alloca when inside a loop (or inside a function that's typically called inside a loop), in order to avoid blowing the stack. But using alloca for resonably- sized objects is otherwise fine. > attachment is the patch to fix above problems I think you need to fold this patch into the previous patches you submitted, and then make sure you re-submit them, one patch per email - making sure you update the ChangeLog files when necessary. I noticed that some of your commits did not have a subject. And it would be good if each commit explained precisely what it improves. Take a look at how each patch in the following series of 4 patches has been submitted for instance: http://www.sourceware.org/ml/gdb-patches/2012-11/msg00854.html (links to referenced emails are at the bottom of the page). I apologize in advance for the comments below, as they seem like nit-picking. But they are about coding style, and trying to be consistent within a project is important. > - stack_dest = (stack_dest + 7) & ~0x7; > + stack_dest = align_down(stack_dest, 8); You are missing a space before the '('. > if (status == 0) { Our style is to put the brace on the next line, thus: if (status == 0) { > /* fix gdb.base/gdb1250 Another nit: Sentences start with a capital letter and end with a period. Thus: /* Fix gdb.base/gdb1250. I am not too fond of references to the testsuite, however. Is that necessary? I don't think people grep the sources when they change anything in the testsuite. As long as you describe why you are making a change, I don't see a need for saying what testcase you are fixing. I haven't really looked at the contents of the patch, only the style. We'll get to that when you resubmit them. > - * breakpoint is set before dynamic library loaded, thus gdb > - * does a partial symbol name finding and sets the breakpoint > - * in the plt stub. our 32-bundle prefetch window is too large > - * for this situation to cause a memory access error. > - * For plt stub, we just need to return directly. > - * > - * x86 does not have this problem, because the first instruction > - * in their plt stub is jump, which ends the analysis also. > + breakpoint is set before dynamic library loaded, thus gdb > + does a partial symbol name finding and sets the breakpoint > + in the plt stub. our 32-bundle prefetch window is too large > + for this situation to cause a memory access error. > + For plt stub, we just need to return directly. > + > + x86 does not have this problem, because the first instruction > + in their plt stub is jump, which ends the analysis also. > */ > - if (strcmp(find_pc_section(instbuf_start)->the_bfd_section->name, > - ".plt") == 0) > + if (in_plt_section (instbuf_start, NULL)) > return instbuf_start; > memory_error (status, next_addr); > } -- Joel