From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10730 invoked by alias); 7 Dec 2012 03:12:44 -0000 Received: (qmail 10720 invoked by uid 22791); 7 Dec 2012 03:12:42 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,KHOP_SPAMHAUS_DROP,KHOP_THREADED,RP_MATCHES_RCVD,TW_EG X-Spam-Check-By: sourceware.org Received: from usmamail.tilera.com (HELO USMAMAIL.TILERA.COM) (12.216.194.151) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 07 Dec 2012 03:12:36 +0000 Received: from localhost.localdomain (124.207.145.166) by USMAExch2.tad.internal.tilera.com (10.3.0.33) with Microsoft SMTP Server (TLS) id 14.0.694.0; Thu, 6 Dec 2012 22:12:34 -0500 Message-ID: <50C15E9C.8010404@tilera.com> Date: Fri, 07 Dec 2012 03:12:00 -0000 From: Jiong Wang User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120615 Thunderbird/13.0.1 MIME-Version: 1.0 To: Joel Brobecker CC: Yao Qi , , Walter Lee , Subject: Re: [PATCH/tilegx] tilegx bug fixes & improvements References: <50C04269.90304@tilera.com> <50C067CC.2050303@codesourcery.com> <50C0B790.9070509@tilera.com> <20121207024144.GB31477@adacore.com> In-Reply-To: <20121207024144.GB31477@adacore.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit 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/msg00129.txt.bz2 Hi Joel & Yao, thanks for your time for careful review & suggestions. I will re-organize the patches, re-test, then re-submit --- Regards, Jiong On 12/07/2012 10:41 AM, Joel Brobecker wrote: >>> 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); >> } >