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

[Joel Brobecker <brobecker@adacore.com>]

> >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