On Wed, 25 Apr 2012, Jakub Jelinek wrote:
> Hi!
>
> This patch fixes the attached execute/ testcases. In some cases
> get_best_mode was ignoring bitregion_* restrictions and returned
> mode that overlapped into adjacent non-bitfields, or store_bit_field_1
> for insv could ignore those restrictions completely.
> After fixing that I've run into several issues that the other parts
> of the patch fix, namely that get_best_mode returning VOIDmode more often
> now may lead to store_split_bit_field being called when it wasn't before,
> and when approaching the bitregion_end point we can't just keep using
> large units (modes), but need to decrease their size in order not to
> overwrite adjacent non-bitfields. The get_best_mode dropping of % align
> change prevents a mutual recursion, e.g. for bitregion_end == 103
> it would prevent using > QImode even far before the bit region boundary,
> even when not touching anything close to that bit region end.
> But the extra
> + && (bitregion_end == 0
> + || bitpos - (bitpos % unit) + unit <= bitregion_end + 1))
> needed to be added after that change, otherwise we'd regress on
> c-c++-common/simulate-thread/bitfields-2.c.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Ok.
Thanks,
Richard.
> 2012-04-25 Jakub Jelinek <ja...@redhat.com>
>
> PR middle-end/52979
> * stor-layout.c (get_best_mode): Don't return mode with bitsize
> larger than maxbits. Don't compute maxbits modulo align.
> Also check that unit bytes long store at bitpos / unit * unit
> doesn't affect bits beyond bitregion_end.
> * expmed.c (store_bit_field_1): Avoid trying insv if OP_MODE MEM
> would not fit into bitregion_start ... bitregion_end + 1 bit
> region.
> (store_split_bit_field): Decrease unit close to end of bitregion_end
> if access is restricted in order to avoid mutual recursion.
>
> * gcc.c-torture/compile/pr52979-1.c: New test.
> * gcc.c-torture/execute/pr52979-1.c: New test.
> * gcc.c-torture/execute/pr52979-2.c: New test.
>
> --- gcc/stor-layout.c.jj 2012-03-26 11:53:20.000000000 +0200
> +++ gcc/stor-layout.c 2012-04-25 10:26:18.881631697 +0200
> @@ -2624,7 +2624,7 @@ get_best_mode (int bitsize, int bitpos,
> if (!bitregion_end)
> maxbits = MAX_FIXED_MODE_SIZE;
> else
> - maxbits = (bitregion_end - bitregion_start) % align + 1;
> + maxbits = bitregion_end - bitregion_start + 1;
>
> /* Find the narrowest integer mode that contains the bit field. */
> for (mode = GET_CLASS_NARROWEST_MODE (MODE_INT); mode != VOIDmode;
> @@ -2645,7 +2645,10 @@ get_best_mode (int bitsize, int bitpos,
> (Though at least one Unix compiler ignores this problem:
> that on the Sequent 386 machine. */
> || MIN (unit, BIGGEST_ALIGNMENT) > align
> - || (largest_mode != VOIDmode && unit > GET_MODE_BITSIZE
> (largest_mode)))
> + || (largest_mode != VOIDmode && unit > GET_MODE_BITSIZE (largest_mode))
> + || unit > maxbits
> + || (bitregion_end
> + && bitpos - (bitpos % unit) + unit > bitregion_end + 1))
> return VOIDmode;
>
> if ((SLOW_BYTE_ACCESS && ! volatilep)
> @@ -2663,7 +2666,9 @@ get_best_mode (int bitsize, int bitpos,
> && unit <= MIN (align, BIGGEST_ALIGNMENT)
> && unit <= maxbits
> && (largest_mode == VOIDmode
> - || unit <= GET_MODE_BITSIZE (largest_mode)))
> + || unit <= GET_MODE_BITSIZE (largest_mode))
> + && (bitregion_end == 0
> + || bitpos - (bitpos % unit) + unit <= bitregion_end + 1))
> wide_mode = tmode;
> }
>
> --- gcc/expmed.c.jj 2012-04-19 11:09:13.000000000 +0200
> +++ gcc/expmed.c 2012-04-24 17:14:34.264897874 +0200
> @@ -640,7 +640,13 @@ store_bit_field_1 (rtx str_rtx, unsigned
> && !(MEM_P (op0) && MEM_VOLATILE_P (op0)
> && flag_strict_volatile_bitfields > 0)
> && ! ((REG_P (op0) || GET_CODE (op0) == SUBREG)
> - && (bitsize + bitpos > GET_MODE_BITSIZE (op_mode))))
> + && (bitsize + bitpos > GET_MODE_BITSIZE (op_mode)))
> + /* Do not use insv if the bit region is restricted and
> + op_mode integer at offset doesn't fit into the
> + restricted region. */
> + && !(MEM_P (op0) && bitregion_end
> + && bitnum - bitpos + GET_MODE_BITSIZE (op_mode)
> + > bitregion_end + 1))
> {
> struct expand_operand ops[4];
> int xbitpos = bitpos;
> @@ -760,7 +766,7 @@ store_bit_field_1 (rtx str_rtx, unsigned
> || GET_MODE_BITSIZE (GET_MODE (op0)) > maxbits
> || (op_mode != MAX_MACHINE_MODE
> && GET_MODE_SIZE (GET_MODE (op0)) > GET_MODE_SIZE (op_mode)))
> - bestmode = get_best_mode (bitsize, bitnum,
> + bestmode = get_best_mode (bitsize, bitnum,
> bitregion_start, bitregion_end,
> MEM_ALIGN (op0),
> (op_mode == MAX_MACHINE_MODE
> @@ -1096,6 +1102,16 @@ store_split_bit_field (rtx op0, unsigned
> offset = (bitpos + bitsdone) / unit;
> thispos = (bitpos + bitsdone) % unit;
>
> + /* When region of bytes we can touch is restricted, decrease
> + UNIT close to the end of the region as needed. */
> + if (bitregion_end
> + && unit > BITS_PER_UNIT
> + && bitpos + bitsdone - thispos + unit > bitregion_end + 1)
> + {
> + unit = unit / 2;
> + continue;
> + }
> +
> /* THISSIZE must not overrun a word boundary. Otherwise,
> store_fixed_bit_field will call us again, and we will mutually
> recurse forever. */
> --- gcc/testsuite/gcc.c-torture/compile/pr52979-1.c.jj 2012-04-25
> 08:27:48.188902802 +0200
> +++ gcc/testsuite/gcc.c-torture/compile/pr52979-1.c 2012-04-25
> 08:27:29.000000000 +0200
> @@ -0,0 +1,15 @@
> +/* PR middle-end/52979 */
> +
> +struct S
> +{
> + unsigned int a : 16, b : 16, c : 16, d : 16, e : 14;
> + unsigned int f : 4, g : 14, h : 8;
> + char i;
> + int j;
> +};
> +
> +void
> +foo (struct S *s)
> +{
> + s->f = 1;
> +}
> --- gcc/testsuite/gcc.c-torture/execute/pr52979-1.c.jj 2012-04-24
> 17:20:51.246662745 +0200
> +++ gcc/testsuite/gcc.c-torture/execute/pr52979-1.c 2012-04-24
> 17:21:03.526586921 +0200
> @@ -0,0 +1,40 @@
> +/* PR middle-end/52979 */
> +
> +extern void abort (void);
> +int c, d, e;
> +
> +void
> +foo (void)
> +{
> +}
> +
> +struct __attribute__((packed)) S { int g : 31; int h : 6; };
> +struct S a = { 1 };
> +static struct S b = { 1 };
> +
> +void
> +bar (void)
> +{
> + a.h = 1;
> + struct S f = { };
> + b = f;
> + e = 0;
> + if (d)
> + c = a.g;
> +}
> +
> +void
> +baz (void)
> +{
> + bar ();
> + a = b;
> +}
> +
> +int
> +main ()
> +{
> + baz ();
> + if (a.g)
> + abort ();
> + return 0;
> +}
> --- gcc/testsuite/gcc.c-torture/execute/pr52979-2.c.jj 2012-04-24
> 17:20:54.603642467 +0200
> +++ gcc/testsuite/gcc.c-torture/execute/pr52979-2.c 2012-04-24
> 17:19:15.000000000 +0200
> @@ -0,0 +1,40 @@
> +/* PR middle-end/52979 */
> +
> +extern void abort (void);
> +int c, d, e;
> +
> +void
> +foo (void)
> +{
> +}
> +
> +struct __attribute__((packed)) S { int g : 31; int h : 6; };
> +static struct S b = { 1 };
> +struct S a = { 1 };
> +
> +void
> +bar (void)
> +{
> + a.h = 1;
> + struct S f = { };
> + b = f;
> + e = 0;
> + if (d)
> + c = a.g;
> +}
> +
> +void
> +baz (void)
> +{
> + bar ();
> + a = b;
> +}
> +
> +int
> +main ()
> +{
> + baz ();
> + if (a.g)
> + abort ();
> + return 0;
> +}
>
> Jakub
>
>
--
Richard Guenther <rguent...@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer
