Re: [PATCH/tilegx] tilegx bug fixes & improvements

[Jiong Wang <jiwang@tilera.com>]

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);
>>             }
>