* [PATCH][genattrtab] Fix memory corruption, allocate enough memory for all bypassed reservations
@ 2014-06-13 9:56 Kyrill Tkachov
2014-06-13 15:51 ` Jeff Law
0 siblings, 1 reply; 6+ messages in thread
From: Kyrill Tkachov @ 2014-06-13 9:56 UTC (permalink / raw)
To: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]
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.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: genattrtab-bypasses.patch --]
[-- Type: text/x-patch; name=genattrtab-bypasses.patch, Size: 1263 bytes --]
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;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH][genattrtab] Fix memory corruption, allocate enough memory for all bypassed reservations 2014-06-13 9:56 [PATCH][genattrtab] Fix memory corruption, allocate enough memory for all bypassed reservations Kyrill Tkachov @ 2014-06-13 15:51 ` Jeff Law 2014-06-16 10:13 ` Kyrill Tkachov 0 siblings, 1 reply; 6+ messages in thread From: Jeff Law @ 2014-06-13 15:51 UTC (permalink / raw) To: Kyrill Tkachov, GCC Patches 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][genattrtab] Fix memory corruption, allocate enough memory for all bypassed reservations 2014-06-13 15:51 ` Jeff Law @ 2014-06-16 10:13 ` Kyrill Tkachov 2014-06-16 16:40 ` Jeff Law 0 siblings, 1 reply; 6+ messages in thread From: Kyrill Tkachov @ 2014-06-16 10:13 UTC (permalink / raw) To: Jeff Law, GCC Patches [-- Attachment #1: Type: text/plain, Size: 3481 bytes --] On 13/06/14 16:51, Jeff Law wrote: > 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? 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 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: genattrtab-bypasses.patch --] [-- Type: text/x-patch; name=genattrtab-bypasses.patch, Size: 1443 bytes --] 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; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][genattrtab] Fix memory corruption, allocate enough memory for all bypassed reservations 2014-06-16 10:13 ` Kyrill Tkachov @ 2014-06-16 16:40 ` Jeff Law 2014-06-17 8:12 ` Kyrill Tkachov 0 siblings, 1 reply; 6+ messages in thread From: Jeff Law @ 2014-06-16 16:40 UTC (permalink / raw) To: Kyrill Tkachov, GCC Patches 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][genattrtab] Fix memory corruption, allocate enough memory for all bypassed reservations 2014-06-16 16:40 ` Jeff Law @ 2014-06-17 8:12 ` Kyrill Tkachov 2014-06-17 20:26 ` Jeff Law 0 siblings, 1 reply; 6+ messages in thread From: Kyrill Tkachov @ 2014-06-17 8:12 UTC (permalink / raw) To: Jeff Law, GCC Patches 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 > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][genattrtab] Fix memory corruption, allocate enough memory for all bypassed reservations 2014-06-17 8:12 ` Kyrill Tkachov @ 2014-06-17 20:26 ` Jeff Law 0 siblings, 0 replies; 6+ messages in thread From: Jeff Law @ 2014-06-17 20:26 UTC (permalink / raw) To: Kyrill Tkachov, GCC Patches 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-06-17 20:26 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-06-13 9:56 [PATCH][genattrtab] Fix memory corruption, allocate enough memory for all bypassed reservations Kyrill Tkachov 2014-06-13 15:51 ` Jeff Law 2014-06-16 10:13 ` Kyrill Tkachov 2014-06-16 16:40 ` Jeff Law 2014-06-17 8:12 ` Kyrill Tkachov 2014-06-17 20:26 ` Jeff Law
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).