Re: [patch] combine ICE fix

[Jeff Law <law at redhat dot com>]

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