[PATCH] Fix incorrect folding of bitfield in a union on big endian target

[PATCH] Fix incorrect folding of bitfield in a union on big endian target

[Thomas Preud'homme]

In the code dealing with folding of structure and union initialization, there is a
check that the size of the constructor is the same as the field being read.
However, in the case of bitfield this test can be wrong because it relies on
TYPE_SIZE to get the size of the field being read but TYPE_SIZE returns the
size of the enclosing integer in that case. This patch also check the size
parameter which contains the actual size of the field being read.

The patch was tested by running the testsuite with three different builds of GCC:
1) bootstrap of GCC on x86_64-linux-gnu
2) arm-none-eabi cross compiler (defaulting to little endian) with testsuite
run under qemu emulqting a Cortex M4
3) arm-none-eabi cross compiler (defaulting to big endian, thanks to patch at [1])
with testsuite run under qemu emulating a Cortex M3.

[1] https://sourceware.org/ml/binutils/2014-08/msg00014.html

No regression were observed on any of the tests. The ChangeLog is as follows:


2014-08-11  Thomas Preud'homme  <thomas.preudhomme@arm.com>

	* gimple-fold.c (fold_ctor_reference): Don't fold in presence of
	bitfields, that is when size doesn't match the size of type or the
	size of the constructor.


diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index 3dcb576..6270c34 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -3099,7 +3099,9 @@ fold_ctor_reference (tree type, tree ctor, unsigned HOST_WIDE_INT offset,
   if (!AGGREGATE_TYPE_P (TREE_TYPE (ctor)) && !offset
       /* VIEW_CONVERT_EXPR is defined only for matching sizes.  */
       && operand_equal_p (TYPE_SIZE (type),
-			  TYPE_SIZE (TREE_TYPE (ctor)), 0))
+			  TYPE_SIZE (TREE_TYPE (ctor)), 0)
+      && !compare_tree_int (TYPE_SIZE (type), size)
+      && !compare_tree_int (TYPE_SIZE (TREE_TYPE (ctor)), size))
     {
       ret = canonicalize_constructor_val (unshare_expr (ctor), from_decl);
       ret = fold_unary (VIEW_CONVERT_EXPR, type, ret);
diff --git a/gcc/testsuite/gcc.c-torture/execute/bitfld-6.c b/gcc/testsuite/gcc.c-torture/execute/bitfld-6.c
new file mode 100644
index 0000000..50927dc
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/bitfld-6.c
@@ -0,0 +1,23 @@
+union U
+{
+  const int a;
+  unsigned b : 20;
+};
+
+static union U u = { 0x12345678 };
+
+/* Constant folding used to fail to account for endianness when folding a
+   union.  */
+
+int
+main (void)
+{
+#ifdef __BYTE_ORDER__
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+  return u.b - 0x45678;
+#else
+  return u.b - 0x12345;
+#endif
+#endif
+  return 0;
+}

Is it ok for trunk?

Best regards,

Thomas

RE: [PATCH] Fix incorrect folding of bitfield in a union on big endian target

[Thomas Preud'homme]

> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@gcc.gnu.org] On Behalf Of Thomas Preud'homme
> 
> No regression were observed on any of the tests. The ChangeLog is as
> follows:
> 
> 
> 2014-08-11  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> 
> 	* gimple-fold.c (fold_ctor_reference): Don't fold in presence of
> 	bitfields, that is when size doesn't match the size of type or the
> 	size of the constructor.

The ChangeLog is imcomplete. This was for gcc/ChangeLog.

For gcc/testsuite/ChangeLog, there is:

2014-07-02  Thomas Preud'homme  <thomas.preudhomme@arm.com>

	* gcc.c-torture/execute/bitfld-6.c: New test.

Best regards,

Thomas 

Re: [PATCH] Fix incorrect folding of bitfield in a union on big endian target

[Mikael Pettersson]

Thomas Preud'homme writes:
 > In the code dealing with folding of structure and union initialization, there is a
 > check that the size of the constructor is the same as the field being read.
 > However, in the case of bitfield this test can be wrong because it relies on
 > TYPE_SIZE to get the size of the field being read but TYPE_SIZE returns the
 > size of the enclosing integer in that case. This patch also check the size
 > parameter which contains the actual size of the field being read.
 > 
 > The patch was tested by running the testsuite with three different builds of GCC:
 > 1) bootstrap of GCC on x86_64-linux-gnu
 > 2) arm-none-eabi cross compiler (defaulting to little endian) with testsuite
 > run under qemu emulqting a Cortex M4
 > 3) arm-none-eabi cross compiler (defaulting to big endian, thanks to patch at [1])
 > with testsuite run under qemu emulating a Cortex M3.
 > 
 > [1] https://sourceware.org/ml/binutils/2014-08/msg00014.html
 > 
 > No regression were observed on any of the tests. The ChangeLog is as follows:
 > 
 > 
 > 2014-08-11  Thomas Preud'homme  <thomas.preudhomme@arm.com>
 > 
 > 	* gimple-fold.c (fold_ctor_reference): Don't fold in presence of
 > 	bitfields, that is when size doesn't match the size of type or the
 > 	size of the constructor.
 > 
 > 
 > diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
 > index 3dcb576..6270c34 100644
 > --- a/gcc/gimple-fold.c
 > +++ b/gcc/gimple-fold.c
 > @@ -3099,7 +3099,9 @@ fold_ctor_reference (tree type, tree ctor, unsigned HOST_WIDE_INT offset,
 >    if (!AGGREGATE_TYPE_P (TREE_TYPE (ctor)) && !offset
 >        /* VIEW_CONVERT_EXPR is defined only for matching sizes.  */
 >        && operand_equal_p (TYPE_SIZE (type),
 > -			  TYPE_SIZE (TREE_TYPE (ctor)), 0))
 > +			  TYPE_SIZE (TREE_TYPE (ctor)), 0)
 > +      && !compare_tree_int (TYPE_SIZE (type), size)
 > +      && !compare_tree_int (TYPE_SIZE (TREE_TYPE (ctor)), size))
 >      {
 >        ret = canonicalize_constructor_val (unshare_expr (ctor), from_decl);
 >        ret = fold_unary (VIEW_CONVERT_EXPR, type, ret);
 > diff --git a/gcc/testsuite/gcc.c-torture/execute/bitfld-6.c b/gcc/testsuite/gcc.c-torture/execute/bitfld-6.c
 > new file mode 100644
 > index 0000000..50927dc
 > --- /dev/null
 > +++ b/gcc/testsuite/gcc.c-torture/execute/bitfld-6.c
 > @@ -0,0 +1,23 @@
 > +union U
 > +{
 > +  const int a;
 > +  unsigned b : 20;
 > +};
 > +
 > +static union U u = { 0x12345678 };
 > +
 > +/* Constant folding used to fail to account for endianness when folding a
 > +   union.  */
 > +
 > +int
 > +main (void)
 > +{
 > +#ifdef __BYTE_ORDER__
 > +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
 > +  return u.b - 0x45678;
 > +#else
 > +  return u.b - 0x12345;
 > +#endif
 > +#endif
 > +  return 0;
 > +}
 > 
 > Is it ok for trunk?
 > 
 > Best regards,
 > 
 > Thomas
 > 

This test case also fails on powerpc64 (BE not LE), sparc64, and m68k,
with all current gcc branches (4.10, 4.9, 4.8).  On my powerpc64 box
the failure started with gcc-4.6; gcc-4.5 back to gcc-3.4.6 seem Ok.

Perhaps give this a proper PR and consider backporting?

/Mikael

Re: [PATCH] Fix incorrect folding of bitfield in a union on big endian target

[Richard Biener]

On Mon, Aug 11, 2014 at 9:36 AM, Thomas Preud'homme
<thomas.preudhomme@arm.com> wrote:
> In the code dealing with folding of structure and union initialization, there is a
> check that the size of the constructor is the same as the field being read.
> However, in the case of bitfield this test can be wrong because it relies on
> TYPE_SIZE to get the size of the field being read but TYPE_SIZE returns the
> size of the enclosing integer in that case. This patch also check the size
> parameter which contains the actual size of the field being read.
>
> The patch was tested by running the testsuite with three different builds of GCC:
> 1) bootstrap of GCC on x86_64-linux-gnu
> 2) arm-none-eabi cross compiler (defaulting to little endian) with testsuite
> run under qemu emulqting a Cortex M4
> 3) arm-none-eabi cross compiler (defaulting to big endian, thanks to patch at [1])
> with testsuite run under qemu emulating a Cortex M3.
>
> [1] https://sourceware.org/ml/binutils/2014-08/msg00014.html
>
> No regression were observed on any of the tests. The ChangeLog is as follows:
>
>
> 2014-08-11  Thomas Preud'homme  <thomas.preudhomme@arm.com>
>
>         * gimple-fold.c (fold_ctor_reference): Don't fold in presence of
>         bitfields, that is when size doesn't match the size of type or the
>         size of the constructor.
>
>
> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
> index 3dcb576..6270c34 100644
> --- a/gcc/gimple-fold.c
> +++ b/gcc/gimple-fold.c
> @@ -3099,7 +3099,9 @@ fold_ctor_reference (tree type, tree ctor, unsigned HOST_WIDE_INT offset,
>    if (!AGGREGATE_TYPE_P (TREE_TYPE (ctor)) && !offset
>        /* VIEW_CONVERT_EXPR is defined only for matching sizes.  */
>        && operand_equal_p (TYPE_SIZE (type),
> -                         TYPE_SIZE (TREE_TYPE (ctor)), 0))
> +                         TYPE_SIZE (TREE_TYPE (ctor)), 0)
> +      && !compare_tree_int (TYPE_SIZE (type), size)
> +      && !compare_tree_int (TYPE_SIZE (TREE_TYPE (ctor)), size))

That's now extra compares (the operand_equal_p check now does
a check that can be derived transitively).

So - ok with the operand_equal_p cehck removed.

Also see if this can be backported.

Thanks,
Richard.

>      {
>        ret = canonicalize_constructor_val (unshare_expr (ctor), from_decl);
>        ret = fold_unary (VIEW_CONVERT_EXPR, type, ret);
> diff --git a/gcc/testsuite/gcc.c-torture/execute/bitfld-6.c b/gcc/testsuite/gcc.c-torture/execute/bitfld-6.c
> new file mode 100644
> index 0000000..50927dc
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/bitfld-6.c
> @@ -0,0 +1,23 @@
> +union U
> +{
> +  const int a;
> +  unsigned b : 20;
> +};
> +
> +static union U u = { 0x12345678 };
> +
> +/* Constant folding used to fail to account for endianness when folding a
> +   union.  */
> +
> +int
> +main (void)
> +{
> +#ifdef __BYTE_ORDER__
> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +  return u.b - 0x45678;
> +#else
> +  return u.b - 0x12345;
> +#endif
> +#endif
> +  return 0;
> +}
>
> Is it ok for trunk?
>
> Best regards,
>
> Thomas
>
>

RE: [PATCH] Fix incorrect folding of bitfield in a union on big endian target

[Thomas Preud'homme]

> From: Richard Biener [mailto:richard.guenther@gmail.com]
> Sent: Monday, August 11, 2014 8:29 PM
> 
> That's now extra compares (the operand_equal_p check now does
> a check that can be derived transitively).
> 
> So - ok with the operand_equal_p cehck removed.
> 
> Also see if this can be backported.

I checked and the bug is present for in both GCC 4.8 and 4.9 branches. The
patch applies cleanly in both cases as well. I did the same testing as for trunk
on the GCC 4.8 branch, that is:

1) bootstrap of GCC on x86_64-linux-gnu
2) arm-none-eabi cross compiler (defaulting to little endian) with testsuite
run under qemu emulqting a Cortex M4
3) arm-none-eabi cross compiler (defaulting to big endian, thanks to patch at [1])
with testsuite run under qemu emulating a Cortex M3.

I'm going to do the same testing for GCC 4.9 now. May I commit the backports?

Best regards,

Thomas

Re: [PATCH] Fix incorrect folding of bitfield in a union on big endian target

[Jakub Jelinek]

On Wed, Aug 13, 2014 at 04:21:24PM +0800, Thomas Preud'homme wrote:
> > From: Richard Biener [mailto:richard.guenther@gmail.com]
> > Sent: Monday, August 11, 2014 8:29 PM
> > 
> > That's now extra compares (the operand_equal_p check now does
> > a check that can be derived transitively).
> > 
> > So - ok with the operand_equal_p cehck removed.
> > 
> > Also see if this can be backported.
> 
> I checked and the bug is present for in both GCC 4.8 and 4.9 branches. The
> patch applies cleanly in both cases as well. I did the same testing as for trunk
> on the GCC 4.8 branch, that is:
> 
> 1) bootstrap of GCC on x86_64-linux-gnu
> 2) arm-none-eabi cross compiler (defaulting to little endian) with testsuite
> run under qemu emulqting a Cortex M4
> 3) arm-none-eabi cross compiler (defaulting to big endian, thanks to patch at [1])
> with testsuite run under qemu emulating a Cortex M3.
> 
> I'm going to do the same testing for GCC 4.9 now. May I commit the backports?

Ok.

	Jakub