[IRA] Fix a function accessing beyond end-of-array

Hello,
I discussed this problem with Vlad in 
http://gcc.gnu.org/ml/gcc/2011-05/msg00131.html. I propose the following 
patch to fix it. Okay to commit?

Revised the ChangeLog.

Thanks
Hari

ChangeLog:
         * ira.c (clarify_prohibited_class_mode_regs): Prevent the 
function from accessing beyond the end of REGNO_REG_CLASS array by 
stopping the loop early.

Patch:
            for (nregs-- ;nregs >= 0; nregs--)
              if (((enum reg_class) pclass

On 11/05/11 19:32, Vladimir Makarov wrote:
> On 05/11/2011 10:07 AM, Hari Sandanagobalane wrote:
>> Hello,
>> I discussed this problem with Vlad in
>> http://gcc.gnu.org/ml/gcc/2011-05/msg00131.html. I propose the
>> following patch to fix it. Okay to commit?
>>
>> Revised the ChangeLog.
>>
>> Thanks
>> Hari
>>
>> ChangeLog:
>>          * ira.c (clarify_prohibited_class_mode_regs): Prevent the
>> function from accessing beyond the end of REGNO_REG_CLASS array by
>> stopping the loop early.
>>
>> Patch:
>> Index: gcc/ira.c
>> ===================================================================
>> --- gcc/ira.c   (revision 173654)
>> +++ gcc/ira.c   (working copy)
>> @@ -1422,6 +1422,12 @@
>>             if (TEST_HARD_REG_BIT
>> (ira_prohibited_class_mode_regs[cl][j], hard_regno))
>>               continue;
>>             nregs = hard_regno_nregs[hard_regno][j];
>> +          if (hard_regno + nregs>= FIRST_PSEUDO_REGISTER)
>
> I think it should be '>' not'>='

Vlad,

The REGNO_REG_CLASS is generally an array of size FIRST_PSEUDO_REGISTER. 
So, the indexes go from 0 to FIRST_PSEUDO_REGISTER-1. I think the ">=" 
condition is fine in that case. Do you agree?

Cheers
Hari

>
> With this change, the patch is ok.  Thanks for the patch.
>
>> +            {
>> +              SET_HARD_REG_BIT (ira_prohibited_class_mode_regs[cl][j],
>> +                                hard_regno);
>> +               continue;
>> +            }
>>             pclass = ira_pressure_class_translate[REGNO_REG_CLASS
>> (hard_regno)];
>>             for (nregs-- ;nregs>= 0; nregs--)
>>               if (((enum reg_class) pclass
>

On Thu, May 12, 2011 at 10:09:42AM +0100, Hari Sandanagobalane wrote:
> The REGNO_REG_CLASS is generally an array of size
> FIRST_PSEUDO_REGISTER. So, the indexes go from 0 to
> FIRST_PSEUDO_REGISTER-1.

That is true.

> I think the ">=" condition is fine in that
> case. Do you agree?

That is wrong.  It is perfectly fine to handle hard reg with
regno (FIRST_PSEUDO_REGISTER - 1) if it has nregs 1, or
hard reg with regno (FIRST_PSEUDO_REGISTER - 2) if it has nregs 1 or 2.
The following loop starts with nregs--, therefore for
hard_regno + nregs == FIRST_PSEUDO_REGISTER at the place of your test
REGNO_REG_CLASS will be used in the loop for hard_regno + nregs - 1
down to hard_regno + 0.

> >
> >With this change, the patch is ok.  Thanks for the patch.
> >
> >>+            {
> >>+              SET_HARD_REG_BIT (ira_prohibited_class_mode_regs[cl][j],
> >>+                                hard_regno);
> >>+               continue;
> >>+            }
> >>            pclass = ira_pressure_class_translate[REGNO_REG_CLASS
> >>(hard_regno)];
> >>            for (nregs-- ;nregs>= 0; nregs--)
> >>              if (((enum reg_class) pclass
> >

	Jakub

On 12/05/11 10:18, Jakub Jelinek wrote:
> On Thu, May 12, 2011 at 10:09:42AM +0100, Hari Sandanagobalane wrote:
>> The REGNO_REG_CLASS is generally an array of size
>> FIRST_PSEUDO_REGISTER. So, the indexes go from 0 to
>> FIRST_PSEUDO_REGISTER-1.
>
> That is true.
>
>> I think the ">=" condition is fine in that
>> case. Do you agree?
>
> That is wrong.  It is perfectly fine to handle hard reg with
> regno (FIRST_PSEUDO_REGISTER - 1) if it has nregs 1, or
> hard reg with regno (FIRST_PSEUDO_REGISTER - 2) if it has nregs 1 or 2.
> The following loop starts with nregs--, therefore for
> hard_regno + nregs == FIRST_PSEUDO_REGISTER at the place of your test
> REGNO_REG_CLASS will be used in the loop for hard_regno + nregs - 1
> down to hard_regno + 0.

Ooh yes. You are right. Thanks for that!

I have now committed the revised patch to mainline as revision 173699.

Cheers
Hari

>
>>>
>>> With this change, the patch is ok.  Thanks for the patch.
>>>
>>>> +            {
>>>> +              SET_HARD_REG_BIT (ira_prohibited_class_mode_regs[cl][j],
>>>> +                                hard_regno);
>>>> +               continue;
>>>> +            }
>>>>             pclass = ira_pressure_class_translate[REGNO_REG_CLASS
>>>> (hard_regno)];
>>>>             for (nregs-- ;nregs>= 0; nregs--)
>>>>               if (((enum reg_class) pclass
>>>
>
> 	Jakub