| Message ID | 539ACAD9.7030501@arm.com |
|---|---|
| State | New |
| Headers | show |
On 06/13/14 03:56, Kyrill Tkachov wrote: > Hi all, > > I noticed a memory corruption bug while adding some scheduler bypasses > in the arm backend. > genattrtab would segfault while processing the bypasses. Valgrind > confirmed this. > > The problem is that when processing the bypassed reservations, > make_automaton_pairs allocates memory in proportion to the number of > defined bypasses rather than the number of bypassed reservations. This > means that if the number of bypassed reservations is sufficiently larger > than the number of bypasses, the loop will overwrite unallocated memory. > > I also observed this effect on aarch64, but there was no segfault there, > presumably because the number of reservations in aarch64 is much smaller > than arm at the moment (we only use two pipeline descriptions in aarch64). > > This patch fixes that and valgrind confirms that there's no out of > bounds accesses now. > > Bootstrapped and tested arm-none-linux-gnueabihf, > aarch64-none-linux-gnu, x86_64-linux. > > Ok for trunk? > > Thanks, > Kyrill > > 2014-06-13 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > * genattrtab.c (n_bypassed): New variable. > (process_bypasses): Initialise n_bypassed. > Count number of bypassed reservations. > (make_automaton_attrs): Allocate space for bypassed reservations > rather than number of bypasses. > > genattrtab-bypasses.patch > > > diff --git a/gcc/genattrtab.c b/gcc/genattrtab.c > index c5ce51c..2b6b3ce 100644 > --- a/gcc/genattrtab.c > +++ b/gcc/genattrtab.c > @@ -4766,6 +4766,7 @@ struct bypass_list > > static struct bypass_list *all_bypasses; > static size_t n_bypasses; > +static size_t n_bypassed; > > static void > gen_bypass_1 (const char *s, size_t len) > @@ -4811,12 +4812,19 @@ process_bypasses (void) > struct bypass_list *b; > struct insn_reserv *r; > > + n_bypassed = 0; > + > /* The reservation list is likely to be much longer than the bypass > list. */ > for (r = all_insn_reservs; r; r = r->next) > for (b = all_bypasses; b; b = b->next) > if (fnmatch (b->pattern, r->name, 0) == 0) > - r->bypassed = true; > + { > + if (!r->bypassed) > + n_bypassed++; > + > + r->bypassed = true; > + } > } Might as well go ahead and put the r->bypassed = true assignment inside the if (!r->bypassed) conditional. It probably doesn't matter in terms of real performance, but it's easy to do. In fact, once you hit that case ISTM the inner loop is pointless. So I think it ought to look like: /* The reservation list is likely to be much longer than the bypass list. */ for (r = all_insn_reservs; r; r = r->next) for (b = all_bypasses; b; b = b->next) if (fnmatch (b->pattern, r->name, 0) == 0) { r->bypassed = true; n_bypassed++; break; } Or am I missing something? Jeff
diff --git a/gcc/genattrtab.c b/gcc/genattrtab.c index c5ce51c..2b6b3ce 100644 --- a/gcc/genattrtab.c +++ b/gcc/genattrtab.c @@ -4766,6 +4766,7 @@ struct bypass_list static struct bypass_list *all_bypasses; static size_t n_bypasses; +static size_t n_bypassed; static void gen_bypass_1 (const char *s, size_t len) @@ -4811,12 +4812,19 @@ process_bypasses (void) struct bypass_list *b; struct insn_reserv *r; + n_bypassed = 0; + /* The reservation list is likely to be much longer than the bypass list. */ for (r = all_insn_reservs; r; r = r->next) for (b = all_bypasses; b; b = b->next) if (fnmatch (b->pattern, r->name, 0) == 0) - r->bypassed = true; + { + if (!r->bypassed) + n_bypassed++; + + r->bypassed = true; + } } /* Check that attribute NAME is used in define_insn_reservation condition @@ -5075,7 +5083,7 @@ make_automaton_attrs (void) process_bypasses (); byps_exp = rtx_alloc (COND); - XVEC (byps_exp, 0) = rtvec_alloc (n_bypasses * 2); + XVEC (byps_exp, 0) = rtvec_alloc (n_bypassed * 2); XEXP (byps_exp, 1) = make_numeric_value (0); for (decl = all_insn_reservs, i = 0; decl;
Hi all, I noticed a memory corruption bug while adding some scheduler bypasses in the arm backend. genattrtab would segfault while processing the bypasses. Valgrind confirmed this. The problem is that when processing the bypassed reservations, make_automaton_pairs allocates memory in proportion to the number of defined bypasses rather than the number of bypassed reservations. This means that if the number of bypassed reservations is sufficiently larger than the number of bypasses, the loop will overwrite unallocated memory. I also observed this effect on aarch64, but there was no segfault there, presumably because the number of reservations in aarch64 is much smaller than arm at the moment (we only use two pipeline descriptions in aarch64). This patch fixes that and valgrind confirms that there's no out of bounds accesses now. Bootstrapped and tested arm-none-linux-gnueabihf, aarch64-none-linux-gnu, x86_64-linux. Ok for trunk? Thanks, Kyrill 2014-06-13 Kyrylo Tkachov <kyrylo.tkachov@arm.com> * genattrtab.c (n_bypassed): New variable. (process_bypasses): Initialise n_bypassed. Count number of bypassed reservations. (make_automaton_attrs): Allocate space for bypassed reservations rather than number of bypasses.