diff mbox

[RFC] Fix PR48124

Message ID alpine.LNX.2.00.1202021146320.4999@zhemvz.fhfr.qr
State New
Headers show

Commit Message

Richard Biener Feb. 2, 2012, 11:36 a.m. UTC 
On Wed, 1 Feb 2012, Richard Guenther wrote:

> 
> This fixes PR48124 where a bitfield store leaks into adjacent
> decls if the decl we store to has bigger alignment than what
> its type requires (thus there is another decl in that "padding").

[...]

> Bootstraped on x86_64-unknown-linux-gnu, testing in progress.

So testing didn't go too well (which makes me suspicious about
the adjust_address the strict-volatile-bitfield path does ...).

The following patch instead tries to make us honor mode1 as
maximum sized mode to be used for accesses to the bitfield
(mode1 as that returned from get_inner_reference, so in theory
that would cover the strict-volatile-bitfield cases already).

It does so by passing down that mode to store_fixed_bit_field
and use it as max-mode argument to get_best_mode there.  The
patch also checks that the HAVE_insv paths would not use
bigger stores than that mode (hopefully I've covered all cases
where we would do that).

Currently all bitfields (that are not volatile) get VOIDmode
from get_inner_reference and the patch tries hard to not
change things in that case.  The expr.c hunk contains one
possible fix for 48124 by computing mode1 based on MEM_SIZE
(probably not the best way - any good ideas welcome).

The patch should also open up the way for fixing PR52080
(that bitfield-store-clobbers-adjacent-fields bug ...)
by simply making get_inner_reference go the
strict-volatile-bitfield path for all bitfield accesses
(and possibly even the !DECL_BIT_FIELD pieces of it).
Thus, do what people^WLinus would expect for "sane" C
(thus non-mixed base-type bitfields).

I'm looking for comments on both pieces of the patch
(is the strict-volatile-bitfields approach using
adjust-address really correct?  is passing down mode1
as forced maximum-size mode correct, or will it pessimize
code too much?)

Bootstrapped and tested on x86_64-unknown-linux-gnu.

I don't see that we can fix 52080 in full for 4.7 but it
would be nice to at least fix 48124 with something not
too invasive (suggestions for some narrower cases to
adjust mode1 welcome).  Other than MEM_SIZE we could
simply use TYPE_ALIGN if that is less than MEM_ALIGN
and compute a maximum size mode based on that.

Thanks,
Richard.

2012-01-02  Richard Guenther  <rguenther@suse.de>

	* expmed.c (store_bit_field_1): Restrict HAVE_insv handling
	to cases where we can honor an access of at most fieldmode size.
	Pass down fieldmode to store_fixed_bit_field.
	(store_fixed_bit_field): Take maximum mode parameter.  Use
	it instead of word_mode as maximum mode passed to get_best_mode.
	(store_split_bit_field): Call store_fixed_bit_field with
	word_mode as maximum mode.

	PR middle-end/48124
	* expr.c (expand_assignment): Adjust mode1 for bitfields
	to not possibly access tail alignment padding of an object.

	* gcc.dg/torture/pr48124-1.c: New testcase.
	* gcc.dg/torture/pr48124-2.c: Likewise.
	* gcc.dg/torture/pr48124-3.c: Likewise.
	* gcc.dg/torture/pr48124-4.c: Likewise.
diff mbox

Patch

Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 183791)
+++ gcc/expr.c	(working copy)
@@ -4705,6 +4705,17 @@  expand_assignment (tree to, tree from, b
 	    to_rtx = adjust_address (to_rtx, mode1, 0);
 	  else if (GET_MODE (to_rtx) == VOIDmode)
 	    to_rtx = adjust_address (to_rtx, BLKmode, 0);
+	  else if (mode1 == VOIDmode
+		   && MEM_SIZE_KNOWN_P (to_rtx))
+	    {
+	      mode1 = mode_for_size ((MEM_SIZE (to_rtx) & -MEM_SIZE (to_rtx))
+				     * BITS_PER_UNIT, MODE_INT, 0);
+	      if (mode1 == BLKmode
+		  || GET_MODE_SIZE (mode1) > GET_MODE_SIZE (word_mode)
+		  || (GET_MODE_SIZE (mode1)
+		      < GET_MODE_SIZE (DECL_MODE (TREE_OPERAND (to, 1)))))
+		mode1 = VOIDmode;
+	    }
 	}
  
       if (offset != 0)
Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c	(revision 183791)
+++ gcc/expmed.c	(working copy)
@@ -50,7 +50,7 @@  static void store_fixed_bit_field (rtx,
 				   unsigned HOST_WIDE_INT,
 				   unsigned HOST_WIDE_INT,
 				   unsigned HOST_WIDE_INT,
-				   rtx);
+				   rtx, enum machine_mode);
 static void store_split_bit_field (rtx, unsigned HOST_WIDE_INT,
 				   unsigned HOST_WIDE_INT,
 				   unsigned HOST_WIDE_INT,
@@ -632,6 +632,8 @@  store_bit_field_1 (rtx str_rtx, unsigned
       && GET_MODE (value) != BLKmode
       && bitsize > 0
       && GET_MODE_BITSIZE (op_mode) >= bitsize
+      && (fieldmode == VOIDmode
+	  || GET_MODE_BITSIZE (fieldmode) >= GET_MODE_BITSIZE (op_mode))
       /* Do not use insv for volatile bitfields when
          -fstrict-volatile-bitfields is in effect.  */
       && !(MEM_P (op0) && MEM_VOLATILE_P (op0)
@@ -761,7 +763,10 @@  store_bit_field_1 (rtx str_rtx, unsigned
 				  bitregion_start, bitregion_end,
 				  MEM_ALIGN (op0),
 				  (op_mode == MAX_MACHINE_MODE
-				   ? VOIDmode : op_mode),
+				   || (fieldmode != VOIDmode
+				       && (GET_MODE_SIZE (fieldmode)
+					   > GET_MODE_SIZE (op_mode))))
+				  ? fieldmode : op_mode,
 				  MEM_VOLATILE_P (op0));
       else
 	bestmode = GET_MODE (op0);
@@ -802,7 +807,8 @@  store_bit_field_1 (rtx str_rtx, unsigned
     return false;
 
   store_fixed_bit_field (op0, offset, bitsize, bitpos,
-			 bitregion_start, bitregion_end, value);
+			 bitregion_start, bitregion_end, value,
+			 fieldmode);
   return true;
 }
 
@@ -872,7 +878,7 @@  store_fixed_bit_field (rtx op0, unsigned
 		       unsigned HOST_WIDE_INT bitpos,
 		       unsigned HOST_WIDE_INT bitregion_start,
 		       unsigned HOST_WIDE_INT bitregion_end,
-		       rtx value)
+		       rtx value, enum machine_mode wmode)
 {
   enum machine_mode mode;
   unsigned int total_bits = BITS_PER_WORD;
@@ -911,10 +917,13 @@  store_fixed_bit_field (rtx op0, unsigned
 	 a word, we won't be doing the extraction the normal way.
 	 We don't want a mode bigger than the destination.  */
 
+      if (wmode == VOIDmode)
+	wmode = word_mode;
+
       mode = GET_MODE (op0);
       if (GET_MODE_BITSIZE (mode) == 0
-	  || GET_MODE_BITSIZE (mode) > GET_MODE_BITSIZE (word_mode))
-	mode = word_mode;
+	  || GET_MODE_BITSIZE (mode) > GET_MODE_BITSIZE (wmode))
+	mode = wmode;
 
       if (MEM_VOLATILE_P (op0)
           && GET_MODE_BITSIZE (GET_MODE (op0)) > 0
@@ -1169,7 +1178,8 @@  store_split_bit_field (rtx op0, unsigned
 	 it is just an out-of-bounds access.  Ignore it.  */
       if (word != const0_rtx)
 	store_fixed_bit_field (word, offset * unit / BITS_PER_UNIT, thissize,
-			       thispos, bitregion_start, bitregion_end, part);
+			       thispos, bitregion_start, bitregion_end, part,
+			       word_mode);
       bitsdone += thissize;
     }
 }
Index: gcc/testsuite/gcc.dg/torture/pr48124-1.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr48124-1.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr48124-1.c	(revision 0)
@@ -0,0 +1,33 @@ 
+/* { dg-do run } */
+/* { dg-options "-fno-toplevel-reorder" } */
+
+extern void abort (void);
+
+struct S
+{
+  signed a : 26;
+  signed b : 16;
+  signed c : 10;
+  volatile signed d : 14;
+};
+
+static struct S e = { 0, 0, 0, 1 };
+static int f = 1;
+
+void __attribute__((noinline))
+foo (void)
+{
+  e.d = 0;
+  f = 2;
+}
+
+int
+main ()
+{
+  if (e.a || e.b || e.c || e.d != 1 || f != 1)
+    abort ();
+  foo ();
+  if (e.a || e.b || e.c || e.d || f != 2)
+    abort ();
+  return 0;
+}
Index: gcc/testsuite/gcc.dg/torture/pr48124-2.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr48124-2.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr48124-2.c	(revision 0)
@@ -0,0 +1,27 @@ 
+/* { dg-do run } */
+
+extern void abort (void);
+
+static volatile struct S0 {
+    short f3[9];
+    unsigned f8 : 15;
+} s = {1};
+static unsigned short sh = 0x1234;
+
+struct S0 a, b;
+int vi = 0;
+
+void func_4()
+{
+  s.f8 |= 1;
+  sh = 15;
+  if (vi) a = b;
+}
+
+int main()
+{
+  func_4();
+  if (sh != 15)
+    abort ();
+  return 0;
+}
Index: gcc/testsuite/gcc.dg/torture/pr48124-3.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr48124-3.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr48124-3.c	(revision 0)
@@ -0,0 +1,32 @@ 
+/* { dg-do run } */
+
+extern void abort (void);
+struct S1
+{
+  int f0;
+  int:1;
+  int f3;
+  int:1;
+  int:0;
+  int f6:1;
+};
+int g_13 = 1;
+volatile struct S1 g_118 = {
+    1
+};
+
+void __attribute__((noinline))
+func_46 ()
+{
+  for (g_13 = 0; g_13 >= 0; g_13 -= 1)
+    g_118.f6 = 0;
+}
+
+int
+main ()
+{
+  func_46 ();
+  if (g_13 != -1)
+    abort ();
+  return 0;
+}
Index: gcc/testsuite/gcc.dg/torture/pr48124-4.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr48124-4.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr48124-4.c	(revision 0)
@@ -0,0 +1,28 @@ 
+/* { dg-do run } */
+
+extern void abort (void);
+struct S1 {
+    unsigned f0, f1;
+    unsigned short f2, f3;
+    unsigned f4 : 16;
+    unsigned f5, f6;
+    volatile unsigned f7 : 28;
+};
+static struct S1 g_76;
+static struct S1 g_245 = {0,0,0,0,0,0,0,1};
+static signed char g_323 = 0x80;
+static void func_1(void)
+{
+  g_245.f7 &= 1;
+  for (g_323 = 0; g_323 <= -1; g_323 -= 2) {
+      g_76 = g_76;
+      g_76.f4 ^= 11;
+  }
+}
+int main()
+{
+  func_1();
+  if (g_323 != 0 || g_245.f7 != 1)
+    abort ();
+  return 0;
+}