Fix UB in prepare_cmp_insn (PR middle-end/89281)

Hi!

The following hunk of code results in UB on the recently added testcase,
because if cmp_mode is SImode or DImode, then 1 << 32 or 1 << 64 is
undefined.  Fixed by using GET_MODE_MASK, plus UINTVAL because size is
really unsigned (code later on uses unsignedp=1 too).

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2019-02-12  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/89281
	* optabs.c (prepare_cmp_insn): Use UINTVAL (size) instead of
	INTVAL (size), compare it to GET_MODE_MASK instead of
	1 << GET_MODE_BITSIZE.


	Jakub

> The following hunk of code results in UB on the recently added testcase,
> because if cmp_mode is SImode or DImode, then 1 << 32 or 1 << 64 is
> undefined.  Fixed by using GET_MODE_MASK, plus UINTVAL because size is
> really unsigned (code later on uses unsignedp=1 too).

Doesn't the current check make sure that the RTL constant is valid for the 
mode though (since RTL constants are sign-extended for their mode)?  See 
emit_block_move_via_movmem for an equivalent check with GET_MODE_MASK >> 1.

On Wed, Feb 13, 2019 at 12:51:26AM +0100, Eric Botcazou wrote:
> > The following hunk of code results in UB on the recently added testcase,
> > because if cmp_mode is SImode or DImode, then 1 << 32 or 1 << 64 is
> > undefined.  Fixed by using GET_MODE_MASK, plus UINTVAL because size is
> > really unsigned (code later on uses unsignedp=1 too).
> 
> Doesn't the current check make sure that the RTL constant is valid for the 
> mode though (since RTL constants are sign-extended for their mode)?  See 
> emit_block_move_via_movmem for an equivalent check with GET_MODE_MASK >> 1.

The code will do:
          size = convert_to_mode (cmp_mode, size, 1);
i.e. convert size from whatever mode it had before to cmp_mode and the
test is whether it can do so without changing the behavior.  If size is
non-constant, then that can be obviously (without using range info etc.)
done only if the original mode is narrower or at most as wide as cmp_mode.
We could do the same for CONST_INT_P too, but as we know the constant,
it wants to make sure that the size can be expressed in cmp_mode.
As it is unsigned quantity, that can be checked by checking if the value is
<= GET_MODE_MASK.

	Jakub

> The code will do:
>           size = convert_to_mode (cmp_mode, size, 1);
> i.e. convert size from whatever mode it had before to cmp_mode and the
> test is whether it can do so without changing the behavior.  If size is
> non-constant, then that can be obviously (without using range info etc.)
> done only if the original mode is narrower or at most as wide as cmp_mode.
> We could do the same for CONST_INT_P too, but as we know the constant,
> it wants to make sure that the size can be expressed in cmp_mode.
> As it is unsigned quantity, that can be checked by checking if the value is
> <= GET_MODE_MASK.

That's one interpretationn, the other being that of emit_block_move_via_movmem 
and the latter looks sensible to me too: if the top bit is set, the conversion 
will yield a negative RTL constant which will be sent to the target pattern, 
which could not be prepared for such a negative constant:

`movmemM'
     Block move instruction.  The destination and source blocks of
     memory are the first two operands, and both are `mem:BLK's with an
     address in mode `Pmode'.

     The number of bytes to move is the third operand, in mode M.
     Usually, you specify `Pmode' for M.  However, if you can generate
     better code knowing the range of valid lengths is smaller than
     those representable in a full Pmode pointer, you should provide a
     pattern with a mode corresponding to the range of values you can
     handle efficiently (e.g., `QImode' for values in the range 0-127;
     note we avoid numbers that appear negative) and also a pattern
     with `Pmode'.

That being said, your patch doesn't change the interpretation so is OK.