diff mbox

Fill bitregion_{start,end} in store_constructor (PR, tree-optimization/78428).

Message ID 22d70b27-ddad-6410-3c59-1e630d3d7d55@suse.cz
State Superseded
Headers show

Commit Message

Martin Liška Nov. 23, 2016, 10:26 a.m. UTC
Following patch fixes situation where we do a store to a bitfield which
is at boundary of a record. This leads to usage of wider store, leading
to overwriting a following memory location.

Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
Apart from that, the reported test-case in PR works on x86_64-linux-gnu.

Ready to be installed?
Martin

Comments

Richard Biener Nov. 23, 2016, 10:49 a.m. UTC | #1
On Wed, Nov 23, 2016 at 11:26 AM, Martin Liška <mliska@suse.cz> wrote:
> Following patch fixes situation where we do a store to a bitfield which

> is at boundary of a record. This leads to usage of wider store, leading

> to overwriting a following memory location.

>

> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

> Apart from that, the reported test-case in PR works on x86_64-linux-gnu.

>

> Ready to be installed?


+  HOST_WIDE_INT bitregion_end
+    = exp_size == -1 ? 0 : exp_size * BITS_PER_UNIT - 1;

I don't think looking at the CONSTRUCTOR to determine bitregion_end is
a good idea.
The function gets 'size' as argument which is documented as "number of
bytes we are
allowed to modify" - so better use that.

@@ -6308,7 +6314,8 @@ store_constructor (tree exp, rtx target, int
cleared, HOST_WIDE_INT size,
                MEM_KEEP_ALIAS_SET_P (to_rtx) = 1;
              }

-           store_constructor_field (to_rtx, bitsize, bitpos, mode,
+           store_constructor_field (to_rtx, bitsize, bitpos,
+                                    0, bitregion_end, mode,
                                     value, cleared,
                                     get_alias_set (TREE_TYPE (field)),
                                     reverse);

this stores to to_rtx which may be offsetted from target this means in this case
bitregion_end is not conservative enough - you'd need to resort to the
field width
in that case I guess (and for variable field size not specify any end
-- I suppose
the 'size' store_constructor gets might also be "unknown"?).  But maybe all
the non-constant offset / size cases are "dead code" now that we are in GIMPLE?
Note they likely can only appear from Ada code anyway -- CCing Eric.

I suppose a "safe" thing to do would be to give up on the first
variable offset/size
and re-set bitregion_end to zero for this and all following fields.

The other cases look fine to me.

Thanks,
Richard.

> Martin
Martin Liška Dec. 6, 2016, 5:51 p.m. UTC | #2
Pinging Eric.

On 11/23/2016 11:49 AM, Richard Biener wrote:
> On Wed, Nov 23, 2016 at 11:26 AM, Martin Liška <mliska@suse.cz> wrote:

>> Following patch fixes situation where we do a store to a bitfield which

>> is at boundary of a record. This leads to usage of wider store, leading

>> to overwriting a following memory location.

>>

>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

>> Apart from that, the reported test-case in PR works on x86_64-linux-gnu.

>>

>> Ready to be installed?

> 

> +  HOST_WIDE_INT bitregion_end

> +    = exp_size == -1 ? 0 : exp_size * BITS_PER_UNIT - 1;

> 

> I don't think looking at the CONSTRUCTOR to determine bitregion_end is

> a good idea.

> The function gets 'size' as argument which is documented as "number of

> bytes we are

> allowed to modify" - so better use that.

> 

> @@ -6308,7 +6314,8 @@ store_constructor (tree exp, rtx target, int

> cleared, HOST_WIDE_INT size,

>                 MEM_KEEP_ALIAS_SET_P (to_rtx) = 1;

>               }

> 

> -           store_constructor_field (to_rtx, bitsize, bitpos, mode,

> +           store_constructor_field (to_rtx, bitsize, bitpos,

> +                                    0, bitregion_end, mode,

>                                      value, cleared,

>                                      get_alias_set (TREE_TYPE (field)),

>                                      reverse);

> 

> this stores to to_rtx which may be offsetted from target this means in this case

> bitregion_end is not conservative enough - you'd need to resort to the

> field width

> in that case I guess (and for variable field size not specify any end

> -- I suppose

> the 'size' store_constructor gets might also be "unknown"?).  But maybe all

> the non-constant offset / size cases are "dead code" now that we are in GIMPLE?

> Note they likely can only appear from Ada code anyway -- CCing Eric.

> 

> I suppose a "safe" thing to do would be to give up on the first

> variable offset/size

> and re-set bitregion_end to zero for this and all following fields.

> 

> The other cases look fine to me.

> 

> Thanks,

> Richard.

> 

>> Martin
Eric Botcazou Dec. 6, 2016, 6:02 p.m. UTC | #3
> But maybe all the non-constant offset / size cases are "dead code" now that

> we are in GIMPLE? Note they likely can only appear from Ada code anyway


Possibly, but to be sure put a call to gcc_unreachable where appropriate and 
run the Ada testsuite; if it is clean, then send me the patch and I'll run it 
on a more thorough testsuite.

-- 
Eric Botcazou
diff mbox

Patch

From 69961d69e551d9abe31eb7946d5211b99bbe1479 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Tue, 22 Nov 2016 10:15:42 +0100
Subject: [PATCH] Fill bitregion_{start,end} in store_constructor (PR
 tree-optimization/78428).

gcc/testsuite/ChangeLog:

2016-11-22  Martin Liska  <mliska@suse.cz>
	    Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/78428
	* gcc.dg/tree-ssa/pr78428.c: New test.

gcc/ChangeLog:

2016-11-22  Martin Liska  <mliska@suse.cz>
	    Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/78428
	* expr.c (store_constructor_field): Add new arguments.
	(store_constructor): Fill up store_constructor_field for
	FIELD_DECL of a store_constructor_fielductor.
---
 gcc/expr.c                              | 26 ++++++++++++++++++--------
 gcc/testsuite/gcc.dg/tree-ssa/pr78428.c | 27 +++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 8 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr78428.c

diff --git a/gcc/expr.c b/gcc/expr.c
index fe752fb..50bc7c5 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -80,7 +80,8 @@  static void clear_by_pieces (rtx, unsigned HOST_WIDE_INT, unsigned int);
 static rtx_insn *compress_float_constant (rtx, rtx);
 static rtx get_subtarget (rtx);
 static void store_constructor_field (rtx, unsigned HOST_WIDE_INT,
-				     HOST_WIDE_INT, machine_mode,
+				     HOST_WIDE_INT, unsigned HOST_WIDE_INT,
+				     unsigned HOST_WIDE_INT, machine_mode,
 				     tree, int, alias_set_type, bool);
 static void store_constructor (tree, rtx, int, HOST_WIDE_INT, bool);
 static rtx store_field (rtx, HOST_WIDE_INT, HOST_WIDE_INT,
@@ -6077,7 +6078,10 @@  all_zeros_p (const_tree exp)
 
 static void
 store_constructor_field (rtx target, unsigned HOST_WIDE_INT bitsize,
-			 HOST_WIDE_INT bitpos, machine_mode mode,
+			 HOST_WIDE_INT bitpos,
+			 unsigned HOST_WIDE_INT bitregion_start,
+			 unsigned HOST_WIDE_INT bitregion_end,
+			 machine_mode mode,
 			 tree exp, int cleared,
 			 alias_set_type alias_set, bool reverse)
 {
@@ -6112,8 +6116,8 @@  store_constructor_field (rtx target, unsigned HOST_WIDE_INT bitsize,
 			 reverse);
     }
   else
-    store_field (target, bitsize, bitpos, 0, 0, mode, exp, alias_set, false,
-		 reverse);
+    store_field (target, bitsize, bitpos, bitregion_start, bitregion_end, mode,
+		 exp, alias_set, false, reverse);
 }
 
 
@@ -6148,6 +6152,8 @@  store_constructor (tree exp, rtx target, int cleared, HOST_WIDE_INT size,
 {
   tree type = TREE_TYPE (exp);
   HOST_WIDE_INT exp_size = int_size_in_bytes (type);
+  HOST_WIDE_INT bitregion_end
+    = exp_size == -1 ? 0 : exp_size * BITS_PER_UNIT - 1;
 
   switch (TREE_CODE (type))
     {
@@ -6308,7 +6314,8 @@  store_constructor (tree exp, rtx target, int cleared, HOST_WIDE_INT size,
 		MEM_KEEP_ALIAS_SET_P (to_rtx) = 1;
 	      }
 
-	    store_constructor_field (to_rtx, bitsize, bitpos, mode,
+	    store_constructor_field (to_rtx, bitsize, bitpos,
+				     0, bitregion_end, mode,
 				     value, cleared,
 				     get_alias_set (TREE_TYPE (field)),
 				     reverse);
@@ -6468,7 +6475,8 @@  store_constructor (tree exp, rtx target, int cleared, HOST_WIDE_INT size,
 			  }
 
 			store_constructor_field
-			  (target, bitsize, bitpos, mode, value, cleared,
+			  (target, bitsize, bitpos, 0, bitregion_end,
+			   mode, value, cleared,
 			   get_alias_set (elttype), reverse);
 		      }
 		  }
@@ -6571,7 +6579,8 @@  store_constructor (tree exp, rtx target, int cleared, HOST_WIDE_INT size,
 		    target = copy_rtx (target);
 		    MEM_KEEP_ALIAS_SET_P (target) = 1;
 		  }
-		store_constructor_field (target, bitsize, bitpos, mode, value,
+		store_constructor_field (target, bitsize, bitpos, 0,
+					 bitregion_end, mode, value,
 					 cleared, get_alias_set (elttype),
 					 reverse);
 	      }
@@ -6705,7 +6714,8 @@  store_constructor (tree exp, rtx target, int cleared, HOST_WIDE_INT size,
 		  ? TYPE_MODE (TREE_TYPE (value))
 		  : eltmode;
 		bitpos = eltpos * elt_size;
-		store_constructor_field (target, bitsize, bitpos, value_mode,
+		store_constructor_field (target, bitsize, bitpos, 0,
+					 bitregion_end, value_mode,
 					 value, cleared, alias, reverse);
 	      }
 	  }
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr78428.c b/gcc/testsuite/gcc.dg/tree-ssa/pr78428.c
new file mode 100644
index 0000000..3a9b99c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr78428.c
@@ -0,0 +1,27 @@ 
+/* PR tree-optimization/78428.  */
+/* { dg-options "-O2" } */
+/* { dg-do run } */
+
+struct S0
+{
+  int f2;
+  int f3:16;
+  int f4:18;
+} ;
+
+int a = 5;
+struct S0 b = { 3, 0, 0 };
+static struct S0 global[2] = { { 77, 0, 78 }, { 77, 0, 78 } };
+
+int main ()
+{
+  volatile struct S0 *j;
+  for (; a;)
+    {
+      __builtin_printf ("", b.f2);
+      j = &b;
+      *j = global[1];
+      a--;
+    }
+  return 0;
+}
-- 
2.10.2