Message ID | 539EC327.2040004@arm.com |
---|---|
State | New |
Headers | show |
On 06/16/14 04:12, Kyrill Tkachov wrote: > > Doh, you're right. I did consider it but for some reason thought we > might want to iterate over all of the bypasses anyway. Breaking out > seems good. > > How about this? > Tested on arm and aarch64 and confirmed with valgrind that no out of > bounds accesses occur. > I kicked off an x86_64 bootstrap but don't expect any problems. > > Thanks, > Kyrill > > genattrtab-bypasses.patch > > > commit 676b85f7a7cc1446482334dcaad457ac328875a8 > Author: Kyrylo Tkachov<kyrylo.tkachov@arm.com> > Date: Fri Jun 13 11:09:57 2014 +0100 > > [genattrtab] Fix memory corruption with bypasses I'm an idiot. n_bypassed is used to size the vector, so you do have to walk the entire list. Jeff
On 16/06/14 17:39, Jeff Law wrote: > On 06/16/14 04:12, Kyrill Tkachov wrote: > >> Doh, you're right. I did consider it but for some reason thought we >> might want to iterate over all of the bypasses anyway. Breaking out >> seems good. >> >> How about this? >> Tested on arm and aarch64 and confirmed with valgrind that no out of >> bounds accesses occur. >> I kicked off an x86_64 bootstrap but don't expect any problems. >> >> Thanks, >> Kyrill >> >> genattrtab-bypasses.patch >> >> >> commit 676b85f7a7cc1446482334dcaad457ac328875a8 >> Author: Kyrylo Tkachov<kyrylo.tkachov@arm.com> >> Date: Fri Jun 13 11:09:57 2014 +0100 >> >> [genattrtab] Fix memory corruption with bypasses > I'm an idiot. n_bypassed is used to size the vector, so you do have to > walk the entire list. AFAICS in the loop in process_bypasses we want to count all the reservations which have a bypass matching them. Once a reservation is matched with a bypass it should be safe to break out of the inner loop (over the bypasses), even if two bypasses match a reservation we only want to count the reservation once. So I think the 2nd version of the patch is good Thanks, Kyrill > > Jeff > > > >
On 06/17/14 02:12, Kyrill Tkachov wrote: > > On 16/06/14 17:39, Jeff Law wrote: >> On 06/16/14 04:12, Kyrill Tkachov wrote: >> >>> Doh, you're right. I did consider it but for some reason thought we >>> might want to iterate over all of the bypasses anyway. Breaking out >>> seems good. >>> >>> How about this? >>> Tested on arm and aarch64 and confirmed with valgrind that no out of >>> bounds accesses occur. >>> I kicked off an x86_64 bootstrap but don't expect any problems. >>> >>> Thanks, >>> Kyrill >>> >>> genattrtab-bypasses.patch >>> >>> >>> commit 676b85f7a7cc1446482334dcaad457ac328875a8 >>> Author: Kyrylo Tkachov<kyrylo.tkachov@arm.com> >>> Date: Fri Jun 13 11:09:57 2014 +0100 >>> >>> [genattrtab] Fix memory corruption with bypasses >> I'm an idiot. n_bypassed is used to size the vector, so you do have to >> walk the entire list. > > AFAICS in the loop in process_bypasses we want to count all the > reservations which have a bypass matching them. Once a reservation is > matched with a bypass it should be safe to break out of the inner loop > (over the bypasses), even if two bypasses match a reservation we only > want to count the reservation once. > > So I think the 2nd version of the patch is good OK. APproved. jeff
commit 676b85f7a7cc1446482334dcaad457ac328875a8 Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com> Date: Fri Jun 13 11:09:57 2014 +0100 [genattrtab] Fix memory corruption with bypasses diff --git a/gcc/genattrtab.c b/gcc/genattrtab.c index c5ce51c..9db2ade 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,18 @@ 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; + { + n_bypassed++; + r->bypassed = true; + break; + } } /* Check that attribute NAME is used in define_insn_reservation condition @@ -5075,7 +5082,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;