Re: [patch] combine ICE fix

[Jakub Jelinek <jakub at redhat dot com>]

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