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

2019-02-12 Thread Jakub Jelinek
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  

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.

--- gcc/optabs.c.jj 2019-02-05 10:16:34.533743051 +0100
+++ gcc/optabs.c2019-02-11 09:48:15.514432541 +0100
@@ -3898,7 +3898,7 @@ prepare_cmp_insn (rtx x, rtx y, enum rtx
 
  /* Must make sure the size fits the insn's mode.  */
  if (CONST_INT_P (size)
- ? INTVAL (size) >= (1 << GET_MODE_BITSIZE (cmp_mode))
+ ? UINTVAL (size) > GET_MODE_MASK (cmp_mode)
  : (GET_MODE_BITSIZE (as_a  (GET_MODE (size)))
 > GET_MODE_BITSIZE (cmp_mode)))
continue;

Jakub


Re: [PATCH] Fix UB in prepare_cmp_insn (PR middle-end/89281)

2019-02-12 Thread Eric Botcazou
> 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.

-- 
Eric Botcazou


Re: [PATCH] Fix UB in prepare_cmp_insn (PR middle-end/89281)

2019-02-12 Thread Jakub Jelinek
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


Re: [PATCH] Fix UB in prepare_cmp_insn (PR middle-end/89281)

2019-02-13 Thread Eric Botcazou
> 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.

-- 
Eric Botcazou