Fix undefined behaviour in combine.c:force_to_mode

Submitted by Ulrich Weigand on July 6, 2012, 1:11 p.m.

Details

Message ID 201207061311.q66DBHwE028422@d06av02.portsmouth.uk.ibm.com
State New
Headers show

Commit Message

Ulrich Weigand July 6, 2012, 1:11 p.m.
Hello,

in testing a patch on arm-linux-gnueabihf, I ran into a bootstrap comparison
failure that turned out to be caused by a pre-existing bug in common code,
where combine.c code exposed undefined behaviour.

The problem is this code in force_to_mode when simplifying a LSHIFTRT:

      /* Here we can only do something if the shift count is a constant,
         this shift constant is valid for the host, and we can do arithmetic
         in OP_MODE.  */

      if (CONST_INT_P (XEXP (x, 1))
          && INTVAL (XEXP (x, 1)) < HOST_BITS_PER_WIDE_INT
          && HWI_COMPUTABLE_MODE_P (op_mode))
        {
          rtx inner = XEXP (x, 0);
          unsigned HOST_WIDE_INT inner_mask;

          /* Select the mask of the bits we need for the shift operand.  */
          inner_mask = mask << INTVAL (XEXP (x, 1));


If the source code being compiled contains a shift by a negative constant
shift amount, INTVAL (XEXP (x, 1)) at this point can be negative.  (Note
that this does not necessarily mean that the original program has
undefined behaviour, since that shift could be in a place that is never
actually executed.)

If this is the case, then the last line above will cause undefined
behaviour of GCC itself.

Note that most other places in force_to_mode optimizing shifts already
check for non-negative shift amounts; but in this place the check is
missing.

The following patch adds the check here as well, fixing the undefined
behaviour (and subsequent bootstrap comparison failure) in my test.

Tested on arm-linux-gnueabihf.  OK for mainline?

Bye,
Ulrich

ChangeLog:

	* combine.c (force_to_mode) [LSHIFTRT]: Avoid undefined behaviour
	due to negative shift amount.

Comments

Eric Botcazou July 6, 2012, 5:27 p.m.
> Note that most other places in force_to_mode optimizing shifts already
> check for non-negative shift amounts; but in this place the check is
> missing.
>
> The following patch adds the check here as well, fixing the undefined
> behaviour (and subsequent bootstrap comparison failure) in my test.
>
> Tested on arm-linux-gnueabihf.  OK for mainline?

Sure, but given that there is indeed the same pattern a few lines above, you 
could as well have installed it as obvious.  OK for 4.7 too if you need it.

Patch hide | download patch | download mbox

=== modified file 'gcc/combine.c'
--- gcc/combine.c	2012-02-22 12:22:43 +0000
+++ gcc/combine.c	2012-07-03 19:46:18 +0000
@@ -8432,6 +8432,7 @@ 
 	 in OP_MODE.  */
 
       if (CONST_INT_P (XEXP (x, 1))
+	  && INTVAL (XEXP (x, 1)) >= 0
 	  && INTVAL (XEXP (x, 1)) < HOST_BITS_PER_WIDE_INT
 	  && HWI_COMPUTABLE_MODE_P (op_mode))
 	{