| Message ID | 201203261042.13711.ebotcazou@adacore.com |
|---|---|
| State | New |
| Headers | show |
> Bootstrapped/regtested on x86_64-suse-linux, applied on mainline. Should > it be applied to the release branches as well? > > > 2012-03-26 Eric Botcazou <ebotcazou@adacore.com> > > PR rtl-optimization/52629 > * reload1.c (count_pseudo): Short-circuit common case. > (count_spilled_pseudo): Return early for pseudos without hard regs. > Assert that the pseudo has got a hard reg before manipulating it. RMs, what's your decision?
On Tue, 27 Mar 2012, Eric Botcazou wrote: > > Bootstrapped/regtested on x86_64-suse-linux, applied on mainline. Should > > it be applied to the release branches as well? > > > > > > 2012-03-26 Eric Botcazou <ebotcazou@adacore.com> > > > > PR rtl-optimization/52629 > > * reload1.c (count_pseudo): Short-circuit common case. > > (count_spilled_pseudo): Return early for pseudos without hard regs. > > Assert that the pseudo has got a hard reg before manipulating it. > > RMs, what's your decision? What's the impact of this bug? If it is a wrong-code or ice-on-valid bug then it's ok for the 4.7 branch. Richard.
> What's the impact of this bug? If it is a wrong-code or ice-on-valid > bug then it's ok for the 4.7 branch. Neither, it's an out-of-bounds memory access in reload1.c with apparently no visible effects as it has been introduced with IRA (4.4 series).
Index: reload1.c =================================================================== --- reload1.c (revision 185570) +++ reload1.c (working copy) @@ -1746,11 +1746,12 @@ count_pseudo (int reg) int r = reg_renumber[reg]; int nregs; + /* Ignore spilled pseudo-registers which can be here only if IRA is used. */ + if (ira_conflicts_p && r < 0) + return; + if (REGNO_REG_SET_P (&pseudos_counted, reg) - || REGNO_REG_SET_P (&spilled_pseudos, reg) - /* Ignore spilled pseudo-registers which can be here only if IRA - is used. */ - || (ira_conflicts_p && r < 0)) + || REGNO_REG_SET_P (&spilled_pseudos, reg)) return; SET_REGNO_REG_SET (&pseudos_counted, reg); @@ -1827,12 +1828,17 @@ count_spilled_pseudo (int spilled, int s { int freq = REG_FREQ (reg); int r = reg_renumber[reg]; - int nregs = hard_regno_nregs[r][PSEUDO_REGNO_MODE (reg)]; + int nregs; + + /* Ignore spilled pseudo-registers which can be here only if IRA is used. */ + if (ira_conflicts_p && r < 0) + return; + + gcc_assert (r >= 0); + + nregs = hard_regno_nregs[r][PSEUDO_REGNO_MODE (reg)]; - /* Ignore spilled pseudo-registers which can be here only if IRA is - used. */ - if ((ira_conflicts_p && r < 0) - || REGNO_REG_SET_P (&spilled_pseudos, reg) + if (REGNO_REG_SET_P (&spilled_pseudos, reg) || spilled + spilled_nregs <= r || r + nregs <= spilled) return;
This is an out-of-bounds access in count_spilled_pseudo: we can enter the function for a pseudo without hard reg but we immediately use the hard reg number as an index: static void count_spilled_pseudo (int spilled, int spilled_nregs, int reg) { int freq = REG_FREQ (reg); int r = reg_renumber[reg]; int nregs = hard_regno_nregs[r][PSEUDO_REGNO_MODE (reg)]; The negative R case is only checked a few lines below: /* Ignore spilled pseudo-registers which can be here only if IRA is used. */ if ((ira_conflicts_p && r < 0) || REGNO_REG_SET_P (&spilled_pseudos, reg) || spilled + spilled_nregs <= r || r + nregs <= spilled) return; It turns out that count_pseudo contains very similar (and correct) code so the patch just mimics that code. Bootstrapped/regtested on x86_64-suse-linux, applied on mainline. Should it be applied to the release branches as well? 2012-03-26 Eric Botcazou <ebotcazou@adacore.com> PR rtl-optimization/52629 * reload1.c (count_pseudo): Short-circuit common case. (count_spilled_pseudo): Return early for pseudos without hard regs. Assert that the pseudo has got a hard reg before manipulating it.