| Message ID | 201207061311.q66DBHwE028422@d06av02.portsmouth.uk.ibm.com |
|---|---|
| State | New |
| Headers | show |
> 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.
=== 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)) {
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.