| Message ID | 5256B923.5020206@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Oct 10, 2013 at 07:26:43AM -0700, Cesar Philippidis wrote: > This patch addresses an ICE when building qemu for a Mips target in > Yocto. Both gcc-trunk, gcc-4.8 and all of the targets are potentially > affected. The problem occurs because the instruction combine phase uses > two data structures to keep track of registers, reg_stat and > regstat_n_sets_and_refs, but they potentially end up out of sync; when > combine inserts a new register into reg_stat it does not update > regstat_n_sets_and_refs. Failing to update the latter results in an > occasional segmentation fault. > > Is this OK for trunk and gcc-4.8? If so, please check it in. I tested it > on Mips and x86-64 and no regressions showed up. That looks broken. You leave everything from the last size till the current one uninitialized, so it would only work if combine_split_insns always increments max_reg_num () by at most one. Furthermore, there are macros which should be used to access the fields, and, if the vector is ever going to be resized, supposedly it should be vec.h vector rather than just array. Or perhaps take into account: /* If a pass need to change these values in some magical way or the pass needs to have accurate values for these and is not using incremental df scanning, then it should use REG_N_SETS and REG_N_USES. If the pass is doing incremental scanning then it should be getting the info from DF_REG_DEF_COUNT and DF_REG_USE_COUNT. */ and not use REG_N_SETS etc. but instead the df stuff. > --- gcc/regs.h (revision 421441) > +++ gcc/regs.h (working copy) > @@ -85,6 +85,17 @@ REG_N_SETS (int regno) > return regstat_n_sets_and_refs[regno].sets; > } > > +/* Indexed by n, inserts a new register (REG n). */ > +static inline void > +REG_N_GROW (int regno) > +{ > + regstat_n_sets_and_refs = XRESIZEVEC (struct regstat_n_sets_and_refs_t, > + regstat_n_sets_and_refs, regno+1); > + > + regstat_n_sets_and_refs[regno].sets = 1; > + regstat_n_sets_and_refs[regno].refs = 1; > +} > + > /* Indexed by n, gives number of times (REG n) is set. */ > #define SET_REG_N_SETS(N,V) (regstat_n_sets_and_refs[N].sets = V) > #define INC_REG_N_SETS(N,V) (regstat_n_sets_and_refs[N].sets += V) > Index: gcc/combine.c > =================================================================== > --- gcc/combine.c (revision 421441) > +++ gcc/combine.c (working copy) > @@ -518,7 +518,10 @@ combine_split_insns (rtx pattern, rtx insn) > ret = split_insns (pattern, insn); > nregs = max_reg_num (); > if (nregs > reg_stat.length ()) > - reg_stat.safe_grow_cleared (nregs); > + { > + reg_stat.safe_grow_cleared (nregs); > + REG_N_GROW (nregs); > + } > return ret; > } > Jakub
On 10/10/13 10:25, Jakub Jelinek wrote: > On Thu, Oct 10, 2013 at 07:26:43AM -0700, Cesar Philippidis wrote: >> This patch addresses an ICE when building qemu for a Mips target in >> Yocto. Both gcc-trunk, gcc-4.8 and all of the targets are potentially >> affected. The problem occurs because the instruction combine phase uses >> two data structures to keep track of registers, reg_stat and >> regstat_n_sets_and_refs, but they potentially end up out of sync; when >> combine inserts a new register into reg_stat it does not update >> regstat_n_sets_and_refs. Failing to update the latter results in an >> occasional segmentation fault. >> >> Is this OK for trunk and gcc-4.8? If so, please check it in. I tested it >> on Mips and x86-64 and no regressions showed up. > > That looks broken. You leave everything from the last size till the current > one uninitialized, so it would only work if combine_split_insns > always increments max_reg_num () by at most one. I don't think that assumption is safe. Consider a parallel with a bunch of (clobber (match_scratch)) expressions. > Furthermore, there are macros which should be used to access > the fields, and, if the vector is ever going to be resized, supposedly > it should be vec.h vector rather than just array. > Or perhaps take into account: > /* If a pass need to change these values in some magical way or the > pass needs to have accurate values for these and is not using > incremental df scanning, then it should use REG_N_SETS and > REG_N_USES. If the pass is doing incremental scanning then it > should be getting the info from DF_REG_DEF_COUNT and > DF_REG_USE_COUNT. */ > and not use REG_N_SETS etc. but instead the df stuff. Which begs the question, how exactly is combine utilizing the regstat_n_* structures and is that use even valid for combine? Jeff
On 10/15/13 12:16 PM, Jeff Law wrote: > On 10/10/13 10:25, Jakub Jelinek wrote: >> On Thu, Oct 10, 2013 at 07:26:43AM -0700, Cesar Philippidis wrote: >>> This patch addresses an ICE when building qemu for a Mips target in >>> Yocto. Both gcc-trunk, gcc-4.8 and all of the targets are potentially >>> affected. The problem occurs because the instruction combine phase uses >>> two data structures to keep track of registers, reg_stat and >>> regstat_n_sets_and_refs, but they potentially end up out of sync; when >>> combine inserts a new register into reg_stat it does not update >>> regstat_n_sets_and_refs. Failing to update the latter results in an >>> occasional segmentation fault. >>> >>> Is this OK for trunk and gcc-4.8? If so, please check it in. I tested it >>> on Mips and x86-64 and no regressions showed up. >> >> That looks broken. You leave everything from the last size till the >> current >> one uninitialized, so it would only work if combine_split_insns >> always increments max_reg_num () by at most one. > I don't think that assumption is safe. Consider a parallel with a bunch > of (clobber (match_scratch)) expressions. I address that in the patch posted here <http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00802.html>. Is that still insufficient? >> Furthermore, there are macros which should be used to access >> the fields, and, if the vector is ever going to be resized, supposedly >> it should be vec.h vector rather than just array. >> Or perhaps take into account: >> /* If a pass need to change these values in some magical way or the >> pass needs to have accurate values for these and is not using >> incremental df scanning, then it should use REG_N_SETS and >> REG_N_USES. If the pass is doing incremental scanning then it >> should be getting the info from DF_REG_DEF_COUNT and >> DF_REG_USE_COUNT. */ >> and not use REG_N_SETS etc. but instead the df stuff. > Which begs the question, how exactly is combine utilizing the > regstat_n_* structures and is that use even valid for combine? I'll take a look at that. Cesar
On 10/16/13 09:34, Cesar Philippidis wrote: > On 10/15/13 12:16 PM, Jeff Law wrote: >> On 10/10/13 10:25, Jakub Jelinek wrote: >>> On Thu, Oct 10, 2013 at 07:26:43AM -0700, Cesar Philippidis wrote: >>>> This patch addresses an ICE when building qemu for a Mips target in >>>> Yocto. Both gcc-trunk, gcc-4.8 and all of the targets are potentially >>>> affected. The problem occurs because the instruction combine phase uses >>>> two data structures to keep track of registers, reg_stat and >>>> regstat_n_sets_and_refs, but they potentially end up out of sync; when >>>> combine inserts a new register into reg_stat it does not update >>>> regstat_n_sets_and_refs. Failing to update the latter results in an >>>> occasional segmentation fault. >>>> >>>> Is this OK for trunk and gcc-4.8? If so, please check it in. I tested it >>>> on Mips and x86-64 and no regressions showed up. >>> >>> That looks broken. You leave everything from the last size till the >>> current >>> one uninitialized, so it would only work if combine_split_insns >>> always increments max_reg_num () by at most one. >> I don't think that assumption is safe. Consider a parallel with a bunch >> of (clobber (match_scratch)) expressions. > > I address that in the patch posted here > <http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00802.html>. Is that still > insufficient? Thanks. I wasn't aware of the follow-up. > >>> Furthermore, there are macros which should be used to access >>> the fields, and, if the vector is ever going to be resized, supposedly >>> it should be vec.h vector rather than just array. >>> Or perhaps take into account: >>> /* If a pass need to change these values in some magical way or the >>> pass needs to have accurate values for these and is not using >>> incremental df scanning, then it should use REG_N_SETS and >>> REG_N_USES. If the pass is doing incremental scanning then it >>> should be getting the info from DF_REG_DEF_COUNT and >>> DF_REG_USE_COUNT. */ >>> and not use REG_N_SETS etc. but instead the df stuff. >> Which begs the question, how exactly is combine utilizing the >> regstat_n_* structures and is that use even valid for combine? > > I'll take a look at that. This needs to be resolved before we can go forward with your patch. jeff
On 10/16/13, 11:03 AM, Jeff Law wrote: > On 10/16/13 09:34, Cesar Philippidis wrote: >> On 10/15/13 12:16 PM, Jeff Law wrote: >>> On 10/10/13 10:25, Jakub Jelinek wrote: >>>> On Thu, Oct 10, 2013 at 07:26:43AM -0700, Cesar Philippidis wrote: >>>>> This patch addresses an ICE when building qemu for a Mips target in >>>>> Yocto. Both gcc-trunk, gcc-4.8 and all of the targets are potentially >>>>> affected. The problem occurs because the instruction combine phase >>>>> uses >>>>> two data structures to keep track of registers, reg_stat and >>>>> regstat_n_sets_and_refs, but they potentially end up out of sync; when >>>>> combine inserts a new register into reg_stat it does not update >>>>> regstat_n_sets_and_refs. Failing to update the latter results in an >>>>> occasional segmentation fault. >>>>> >>>>> Is this OK for trunk and gcc-4.8? If so, please check it in. I >>>>> tested it >>>>> on Mips and x86-64 and no regressions showed up. >>>> >>>> That looks broken. You leave everything from the last size till the >>>> current >>>> one uninitialized, so it would only work if combine_split_insns >>>> always increments max_reg_num () by at most one. >>> I don't think that assumption is safe. Consider a parallel with a bunch >>> of (clobber (match_scratch)) expressions. >> >> I address that in the patch posted here >> <http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00802.html>. Is that still >> insufficient? > Thanks. I wasn't aware of the follow-up. > >> >>>> Furthermore, there are macros which should be used to access >>>> the fields, and, if the vector is ever going to be resized, supposedly >>>> it should be vec.h vector rather than just array. >>>> Or perhaps take into account: >>>> /* If a pass need to change these values in some magical way or the >>>> pass needs to have accurate values for these and is not using >>>> incremental df scanning, then it should use REG_N_SETS and >>>> REG_N_USES. If the pass is doing incremental scanning then it >>>> should be getting the info from DF_REG_DEF_COUNT and >>>> DF_REG_USE_COUNT. */ >>>> and not use REG_N_SETS etc. but instead the df stuff. >>> Which begs the question, how exactly is combine utilizing the >>> regstat_n_* structures and is that use even valid for combine? >> >> I'll take a look at that. > This needs to be resolved before we can go forward with your patch. Sorry for the delayed response. I had some time to work on this recently. I looked into adding support for incremental DF scanning from df*.[ch] in combine but there are a couple of problems. First of all, combine does its own DF analysis. It does so because its usage falls under this category (df-core.c): c) If the pass modifies insns several times, this incremental updating may be expensive. Furthermore, combine's DF relies on the DF scanning to be deferred, so the DF_REF_DEF_COUNT values would be off. Eg, calls to SET_INSN_DELETED take place before it updates the notes for those insns. Also, combine has a tendency to undo its changes occasionally. With that in mind, is the patch here <http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00802.html> OK? Otherwise, since combine only uses REG_N_SETS, I was considering adding a reg_n_sets member to struct reg_stat_struct. Is that approach better? Thanks, Cesar
On 11/27/13 17:13, Cesar Philippidis wrote: > > I looked into adding support for incremental DF scanning from df*.[ch] > in combine but there are a couple of problems. First of all, combine > does its own DF analysis. It does so because its usage falls under this > category (df-core.c): > > c) If the pass modifies insns several times, this incremental > updating may be expensive. > > Furthermore, combine's DF relies on the DF scanning to be deferred, so > the DF_REF_DEF_COUNT values would be off. Eg, calls to SET_INSN_DELETED > take place before it updates the notes for those insns. Also, combine > has a tendency to undo its changes occasionally. I think at this stage of the release cycle, converting combine to incremental DF is probably a no-go. However, we should keep it in mind for the future -- while hairy I'd really like to see that happen in the long term. As for moving this issue forward, I think the REG_N_SETs uses in combine are valid/legitimate, so the real question in my mind is whether or not to turn this into a vec or leave it as an array. I'd lean for the former -- turning it into a vec gets us bounds checking which probably would have caught this problem earlier. This would also need a testcase for the testsuite. Presumably with a vec version that won't be hard to create since you should get an out-of-bounds error on the array indexing rather waiting for a segfault. jeff
On Dec 2, 2013, at 10:26 PM, Jeff Law <law@redhat.com> wrote: > On 11/27/13 17:13, Cesar Philippidis wrote: >> >> I looked into adding support for incremental DF scanning from df*.[ch] >> in combine but there are a couple of problems. First of all, combine >> does its own DF analysis. It does so because its usage falls under this >> category (df-core.c): >> >> c) If the pass modifies insns several times, this incremental >> updating may be expensive. >> >> Furthermore, combine's DF relies on the DF scanning to be deferred, so >> the DF_REF_DEF_COUNT values would be off. Eg, calls to SET_INSN_DELETED >> take place before it updates the notes for those insns. Also, combine >> has a tendency to undo its changes occasionally. > I think at this stage of the release cycle, converting combine to incremental DF is probably a no-go. However, we should keep it in mind for the future -- while hairy I'd really like to see that happen in the long term. I think Kenny has some thoughts in this area. I'll cc him to ensure he sees it.
On 12/03/2013 01:52 PM, Mike Stump wrote: > On Dec 2, 2013, at 10:26 PM, Jeff Law <law@redhat.com> wrote: >> On 11/27/13 17:13, Cesar Philippidis wrote: >>> I looked into adding support for incremental DF scanning from df*.[ch] >>> in combine but there are a couple of problems. First of all, combine >>> does its own DF analysis. It does so because its usage falls under this >>> category (df-core.c): >>> >>> c) If the pass modifies insns several times, this incremental >>> updating may be expensive. >>> >>> Furthermore, combine's DF relies on the DF scanning to be deferred, so >>> the DF_REF_DEF_COUNT values would be off. Eg, calls to SET_INSN_DELETED >>> take place before it updates the notes for those insns. Also, combine >>> has a tendency to undo its changes occasionally. >> I think at this stage of the release cycle, converting combine to incremental DF is probably a no-go. However, we should keep it in mind for the future -- while hairy I'd really like to see that happen in the long term. > I think Kenny has some thoughts in this area. I'll cc him to ensure he sees it. it is the tendency to undo things (i would use the word "frequently" rather than) occasionally that kept me from doing this years ago. kenny
On 12/03/13 12:25, Kenneth Zadeck wrote: > On 12/03/2013 01:52 PM, Mike Stump wrote: >> On Dec 2, 2013, at 10:26 PM, Jeff Law <law@redhat.com> wrote: >>> On 11/27/13 17:13, Cesar Philippidis wrote: >>>> I looked into adding support for incremental DF scanning from df*.[ch] >>>> in combine but there are a couple of problems. First of all, combine >>>> does its own DF analysis. It does so because its usage falls under this >>>> category (df-core.c): >>>> >>>> c) If the pass modifies insns several times, this incremental >>>> updating may be expensive. >>>> >>>> Furthermore, combine's DF relies on the DF scanning to be deferred, so >>>> the DF_REF_DEF_COUNT values would be off. Eg, calls to SET_INSN_DELETED >>>> take place before it updates the notes for those insns. Also, combine >>>> has a tendency to undo its changes occasionally. >>> I think at this stage of the release cycle, converting combine to >>> incremental DF is probably a no-go. However, we should keep it in >>> mind for the future -- while hairy I'd really like to see that happen >>> in the long term. >> I think Kenny has some thoughts in this area. I'll cc him to ensure >> he sees it. > it is the tendency to undo things (i would use the word "frequently" > rather than) occasionally that kept me from doing this years ago. Shove a bunch of things together, simplify, then try to recognize the result. If that fails, undo everything. In theory, this could be replaced by making a copy of the original, doing the combination/simplification, then recognition. If successful, then update DF and remove the original I3, if not successful, drop the copy. That avoids the undo nonsense. jeff
On 12/03/2013 02:38 PM, Jeff Law wrote: > On 12/03/13 12:25, Kenneth Zadeck wrote: >> On 12/03/2013 01:52 PM, Mike Stump wrote: >>> On Dec 2, 2013, at 10:26 PM, Jeff Law <law@redhat.com> wrote: >>>> On 11/27/13 17:13, Cesar Philippidis wrote: >>>>> I looked into adding support for incremental DF scanning from >>>>> df*.[ch] >>>>> in combine but there are a couple of problems. First of all, combine >>>>> does its own DF analysis. It does so because its usage falls under >>>>> this >>>>> category (df-core.c): >>>>> >>>>> c) If the pass modifies insns several times, this incremental >>>>> updating may be expensive. >>>>> >>>>> Furthermore, combine's DF relies on the DF scanning to be >>>>> deferred, so >>>>> the DF_REF_DEF_COUNT values would be off. Eg, calls to >>>>> SET_INSN_DELETED >>>>> take place before it updates the notes for those insns. Also, combine >>>>> has a tendency to undo its changes occasionally. >>>> I think at this stage of the release cycle, converting combine to >>>> incremental DF is probably a no-go. However, we should keep it in >>>> mind for the future -- while hairy I'd really like to see that happen >>>> in the long term. >>> I think Kenny has some thoughts in this area. I'll cc him to ensure >>> he sees it. >> it is the tendency to undo things (i would use the word "frequently" >> rather than) occasionally that kept me from doing this years ago. > Shove a bunch of things together, simplify, then try to recognize the > result. If that fails, undo everything. > > In theory, this could be replaced by making a copy of the original, > doing the combination/simplification, then recognition. If successful, > then update DF and remove the original I3, if not successful, drop the > copy. That avoids the undo nonsense. > > jeff that could certainly work.
Index: gcc/regs.h =================================================================== --- gcc/regs.h (revision 421441) +++ gcc/regs.h (working copy) @@ -85,6 +85,17 @@ REG_N_SETS (int regno) return regstat_n_sets_and_refs[regno].sets; } +/* Indexed by n, inserts a new register (REG n). */ +static inline void +REG_N_GROW (int regno) +{ + regstat_n_sets_and_refs = XRESIZEVEC (struct regstat_n_sets_and_refs_t, + regstat_n_sets_and_refs, regno+1); + + regstat_n_sets_and_refs[regno].sets = 1; + regstat_n_sets_and_refs[regno].refs = 1; +} + /* Indexed by n, gives number of times (REG n) is set. */ #define SET_REG_N_SETS(N,V) (regstat_n_sets_and_refs[N].sets = V) #define INC_REG_N_SETS(N,V) (regstat_n_sets_and_refs[N].sets += V) Index: gcc/combine.c =================================================================== --- gcc/combine.c (revision 421441) +++ gcc/combine.c (working copy) @@ -518,7 +518,10 @@ combine_split_insns (rtx pattern, rtx insn) ret = split_insns (pattern, insn); nregs = max_reg_num (); if (nregs > reg_stat.length ()) - reg_stat.safe_grow_cleared (nregs); + { + reg_stat.safe_grow_cleared (nregs); + REG_N_GROW (nregs); + } return ret; }