On 28/02/17 16:29, Richard Biener wrote:
On February 28, 2017 5:00:41 PM GMT+01:00, Jakub Jelinek <ja...@redhat.com> wrote:Hi!The following testcases are miscompiled on little endian targets. The bug occurs if we are merging bitfield stores, there is a signed bitfield with bitlen multiple of BITS_PER_UNIT but not equal to the bitsize of the corresponding mode, the bitfield doesn't start on multiple of BITS_PER_UNIT, we store negative constant to it and store to field overlapping 7 bits above this bitfield is done before store to this bitfield. In particular in the testcases, we have bitfield with bitpos 19, bitlen 24 and store value -5 to it. That means we use byte_size of 5 (SImode bytesize + 1 extra byte for shifting), and we end up with 0xfb, 0xff, 0xff, 0xff, 0x00 bytes in tmpbuf. Next, we find out that there is padding of 1 byte, so decrement byte_size to 4 (again, real byte size of the non-shifted bitfield is 3, but 1 extra byte for shifting), but as bitlen is a multiple of BITS_PER_UNIT, we don't clear anything after those 0xfb, 0xff, 0xff bytes. And then shift_bytes_in_array just shifts all bytes left and we end up with 0xd8, 0xff, 0xff, 0xff when we want 0xd8, 0xff, 0xff, 0x07. Finally these bytes are ored into the real buffer where we've previously cleared the bitrange (which is why having 0xff in the last byte is harmful). Now, if bitlen is not a multiple of BITS_PER_UNIT, we already clear that extra byte plus some bits: - else - clear_bit_region (tmpbuf, bitlen, - byte_size * BITS_PER_UNIT - bitlen); Here byte_size is still the actual byte size + 1 and thus say if instead of bitlen 24 we had bitlen 23, we would have cleared 0xfb, 0xff, 0xff, 0xff, 0x00 tmpbuf to 0xfb, 0xff, 0x7f, 0x00, 0x00. For big endian, my understanding is that we also rely on tmpbuf[byte_size - 1] being 0, but we should not suffer from this problem, we clear it first everywhere, then if (native_encode_expr (expr, tmpbuf, byte_size, 0) == 0) should really just write byte_size - 1 bytes due to the mode (though, I'd say it would be much clearer if we called if (native_encode_expr (expr, tmpbuf, byte_size - 1, 0) == 0) because that is what we want (ok to make this change?). Then the padding adjustment for big-endian is actually tmpbuf += padding, so we move the start of the buffer, and for obvious reason don't want to access any bytes before tmpbuf during shifting. The following patch fixes that, bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk (without or with the above native_encode_expr call change)?With the native encode change is fine with me.
Ok with me as well FWIW. That part of the pass is quite tricky to get right when negative numbers (that are sign-extended) get involved. Thanks, Kyrill
Richard.Other options would be to do there tmpbuf[byte_size - 1] = '\0'; unconditionally (we rely on it everywhere, just in other cases it should be already cleared), or do such clearing inside of the two shift_bytes_in_array* functions. 2017-02-28 Jakub Jelinek <ja...@redhat.com> PR tree-optimization/79737 * gimple-ssa-store-merging.c (encode_tree_to_bitpos): If bitlen is a multiple of BITS_PER_UNIT and !BYTES_BIG_ENDIAN, clear tmpbuf[byte_size - 1]. Formatting fix. (shift_bytes_in_array_right): Formatting fix. * gcc.c-torture/execute/pr79737-1.c: New test. * gcc.c-torture/execute/pr79737-2.c: New test. --- gcc/gimple-ssa-store-merging.c.jj 2017-01-01 12:45:38.000000000 +0100 +++ gcc/gimple-ssa-store-merging.c 2017-02-28 10:34:05.848174576 +0100 @@ -253,9 +253,9 @@ shift_bytes_in_array_right (unsigned cha unsigned prev_carry_over = carry_over; carry_over = ptr[i] & carry_mask; - carry_over <<= (unsigned char) BITS_PER_UNIT - amnt; - ptr[i] >>= amnt; - ptr[i] |= prev_carry_over; + carry_over <<= (unsigned char) BITS_PER_UNIT - amnt; + ptr[i] >>= amnt; + ptr[i] |= prev_carry_over; } } @@ -352,8 +352,9 @@ encode_tree_to_bitpos (tree expr, unsign { unsigned int first_byte = bitpos / BITS_PER_UNIT; tree tmp_int = expr; - bool sub_byte_op_p = (bitlen % BITS_PER_UNIT) || (bitpos % BITS_PER_UNIT) - || mode_for_size (bitlen, MODE_INT, 0) == BLKmode; + bool sub_byte_op_p = ((bitlen % BITS_PER_UNIT) + || (bitpos % BITS_PER_UNIT) + || mode_for_size (bitlen, MODE_INT, 0) == BLKmode); if (!sub_byte_op_p) return (native_encode_expr (tmp_int, ptr + first_byte, total_bytes, 0) @@ -418,25 +419,27 @@ encode_tree_to_bitpos (tree expr, unsign contain a sign bit due to sign-extension). */ unsigned int padding = byte_size - ROUND_UP (bitlen, BITS_PER_UNIT) / BITS_PER_UNIT - 1; - if (padding != 0 - || bitlen % BITS_PER_UNIT != 0) + /* On big-endian the padding is at the 'front' so just skip the initial + bytes. */ + if (BYTES_BIG_ENDIAN) + tmpbuf += padding; + + byte_size -= padding; + + if (bitlen % BITS_PER_UNIT != 0) { - /* On big-endian the padding is at the 'front' so just skip the initial - bytes. */ if (BYTES_BIG_ENDIAN) - tmpbuf += padding; - - byte_size -= padding; - if (bitlen % BITS_PER_UNIT != 0) - { - if (BYTES_BIG_ENDIAN) - clear_bit_region_be (tmpbuf, BITS_PER_UNIT - 1, - BITS_PER_UNIT - (bitlen % BITS_PER_UNIT)); - else - clear_bit_region (tmpbuf, bitlen, - byte_size * BITS_PER_UNIT - bitlen); - } - } + clear_bit_region_be (tmpbuf, BITS_PER_UNIT - 1, + BITS_PER_UNIT - (bitlen % BITS_PER_UNIT)); + else + clear_bit_region (tmpbuf, bitlen, + byte_size * BITS_PER_UNIT - bitlen); + } + /* Left shifting relies on the last byte being clear if bitlen is + a multiple of BITS_PER_UNIT, which might not be clear if + there are padding bytes. */ + else if (!BYTES_BIG_ENDIAN) + tmpbuf[byte_size - 1] = '\0'; /* Clear the bit region in PTR where the bits from TMPBUF will be inserted into. */ --- gcc/testsuite/gcc.c-torture/execute/pr79737-1.c.jj 2017-02-28 10:36:13.678474604 +0100 +++ gcc/testsuite/gcc.c-torture/execute/pr79737-1.c 2017-02-28 10:36:00.861645051 +0100 @@ -0,0 +1,37 @@ +/* PR tree-optimization/79737 */ + +#pragma pack(1) +struct S +{ + int b:18; + int c:1; + int d:24; + int e:15; + int f:14; +} i; +int g, j, k; +static struct S h; + +void +foo () +{ + for (j = 0; j < 6; j++) + k = 0; + for (; k < 3; k++) + { + struct S m = { 5, 0, -5, 9, 5 }; + h = m; + if (g) + i = m; + h.e = 0; + } +} + +int +main () +{ + foo (); + if (h.e != 0) + __builtin_abort (); + return 0; +} --- gcc/testsuite/gcc.c-torture/execute/pr79737-2.c.jj 2017-02-28 10:36:16.993430520 +0100 +++ gcc/testsuite/gcc.c-torture/execute/pr79737-2.c 2017-02-28 10:41:25.217325124 +0100 @@ -0,0 +1,41 @@ +/* PR tree-optimization/79737 */ + +#pragma pack(1) +struct S +{ + int b:18; + int c:1; + int d:24; + int e:15; + int f:14; +} i, j; + +void +foo () +{ + i.e = 0; + i.b = 5; + i.c = 0; + i.d = -5; + i.f = 5; +} + +void +bar () +{ + j.b = 5; + j.c = 0; + j.d = -5; + j.e = 0; + j.f = 5; +} + +int +main () +{ + foo (); + bar (); + asm volatile ("" : : : "memory"); + if (i.b != j.b || i.c != j.c || i.d != j.d || i.e != j.e || i.f != j.f) + __builtin_abort (); +} Jakub
