This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH/tilegx] tilegx bug fixes & improvements
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Jiong Wang <jiwang at tilera dot com>
- Cc: Yao Qi <yao at codesourcery dot com>, gdb-patches at sourceware dot org, Walter Lee <walt at tilera dot com>, palves at redhat dot com
- Date: Fri, 7 Dec 2012 06:41:45 +0400
- Subject: Re: [PATCH/tilegx] tilegx bug fixes & improvements
- References: <50C04269.90304@tilera.com> <50C067CC.2050303@codesourcery.com> <50C0B790.9070509@tilera.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