sel-sched: fix UB in init_regs_for_mode [PR100311]

init_regs_for_mode iterates over all hard regs for the machine to test 
if the reg is OK for the mode, but an arithmetic overflow can lead to 
testing elements beyond the end of the arrays allocated for fixed and 
global registers.  Clearly, if a mode requiring multiple hard regs needs 
one beyond the hard reg set, it can't be valid for that mode.

gcc:
	PR rtl-optimization/100311
	* sel-sched.c (init_regs_for_mode): Skip iteration if multiple hard
	regs are needed and they extend beyond the hard reg set.

OK for master?  What about back-ports?

R.

On Wed, Apr 28, 2021 at 12:04:45PM +0100, Richard Earnshaw wrote:
> init_regs_for_mode iterates over all hard regs for the machine to test if
> the reg is OK for the mode, but an arithmetic overflow can lead to testing
> elements beyond the end of the arrays allocated for fixed and global
> registers.  Clearly, if a mode requiring multiple hard regs needs one beyond
> the hard reg set, it can't be valid for that mode.
> 
> gcc:
> 	PR rtl-optimization/100311
> 	* sel-sched.c (init_regs_for_mode): Skip iteration if multiple hard
> 	regs are needed and they extend beyond the hard reg set.
> 
> OK for master?  What about back-ports?

Shouldn't targetm.hard_regno_mode_ok (cur_reg, mode) be false for such regs?
At least, code where something calls hard_regno_nregs after checking
targetm.hard_regno_mode_ok and then handling the regs is quite common.

Anyway, it is ok for backports if somebody approves it for trunk.

> diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c
> index 3e0265daf28..5e68fd5fda5 100644
> --- a/gcc/sel-sched.c
> +++ b/gcc/sel-sched.c
> @@ -1076,6 +1076,9 @@ init_regs_for_mode (machine_mode mode)
>  
>        nregs = hard_regno_nregs (cur_reg, mode);
>  
> +      if (cur_reg + nregs - 1 >= FIRST_PSEUDO_REGISTER)
> +	continue;
> +
>        for (i = nregs - 1; i >= 0; --i)
>          if (fixed_regs[cur_reg + i]
>                  || global_regs[cur_reg + i]


	Jakub

On 28/04/2021 12:22, Jakub Jelinek via Gcc-patches wrote:
> On Wed, Apr 28, 2021 at 12:04:45PM +0100, Richard Earnshaw wrote:
>> init_regs_for_mode iterates over all hard regs for the machine to test if
>> the reg is OK for the mode, but an arithmetic overflow can lead to testing
>> elements beyond the end of the arrays allocated for fixed and global
>> registers.  Clearly, if a mode requiring multiple hard regs needs one beyond
>> the hard reg set, it can't be valid for that mode.
>>
>> gcc:
>> 	PR rtl-optimization/100311
>> 	* sel-sched.c (init_regs_for_mode): Skip iteration if multiple hard
>> 	regs are needed and they extend beyond the hard reg set.
>>
>> OK for master?  What about back-ports?
> 
> Shouldn't targetm.hard_regno_mode_ok (cur_reg, mode) be false for such regs?
> At least, code where something calls hard_regno_nregs after checking
> targetm.hard_regno_mode_ok and then handling the regs is quite common.
> 

Ah, yes.  I think I've found the problem: the MVE VPR register is marked 
to accept any mode (in practice it can only be used for HImode, so it's 
normally safe) and since that is the last hard register, when 
HARD_REGNO_NREGS doesn't return 1 we run into problems with the buffer 
overrun.

I'm working on a proper fix, but there might be complications that go 
beyond the immediate issue at hand...

We could of course, change the patch below into a checking assert of the 
inverse condition: that would help to catch issues like this.

R.

> Anyway, it is ok for backports if somebody approves it for trunk.
> 
>> diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c
>> index 3e0265daf28..5e68fd5fda5 100644
>> --- a/gcc/sel-sched.c
>> +++ b/gcc/sel-sched.c
>> @@ -1076,6 +1076,9 @@ init_regs_for_mode (machine_mode mode)
>>   
>>         nregs = hard_regno_nregs (cur_reg, mode);
>>   
>> +      if (cur_reg + nregs - 1 >= FIRST_PSEUDO_REGISTER)
>> +	continue;
>> +
>>         for (i = nregs - 1; i >= 0; --i)
>>           if (fixed_regs[cur_reg + i]
>>                   || global_regs[cur_reg + i]
> 
> 
> 	Jakub
>

On Wed, Apr 28, 2021 at 03:06:53PM +0100, Richard Earnshaw wrote:
> We could of course, change the patch below into a checking assert of the
> inverse condition: that would help to catch issues like this.

I have nothing against an assert, but am not sure that sel-sched is the best
spot for that, because it is a non-default scheduling that is only rarely
used.

	Jakub