[gimple] assignments to volatile

[Nathan Sidwell <nathan at codesourcery dot com>]

I've come across inconsistent and surprising behaviour when assigning to 
volatile lvalues.

Sometimes we re-read the assigned-to object, and sometimes we do not.  For instance,
   return vobj = data;
will cause a reread of vobj, IF data is not a constant.
or
  cond ? vobj = data : 0
will cause a reread of vobj, even when we don't use the result of the 
conditional expression -- unless data is a constant, in which case there is 
no-reread.  (And confusingly, if you put void casts inside the two conditional 
sub-expressions, the re-reading goes away, but it doesn't even when you 
explicitly cast the whole expression to void).

In the attached testcase, test_1, test_6 and test_7 have this surprising 
rereading behaviour.

The fault appears to be in gimplify_modify_expr, where we return the LHS as the 
value, if the caller wants the value.  The attached patch changes that routine 
when the target lvalue is volatile.  In that case it will create a temporary to 
hold the RHS value, assign that temporary to the LHS and then return the temporary.

Although the bug is architecture-neutral, the testcase is x86-specific because 
it's checking for specific accesses occurring.

built and tested on i686-pc-linux-gnu, ok?

nathan
--
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery

2010-06-21  Nathan Sidwell  <nathan@codesourcery.com>

	gcc/
	* gimplify.c (gimplify_modify_expr): When assigning to volatiles,
	copy the src value and return a copy.

	gcc/testsuite/
	* gcc.target/i386/volatile-2.c: New.

Index: gimplify.c
===================================================================
--- gimplify.c	(revision 160876)
+++ gimplify.c	(working copy)
@@ -4467,6 +4467,28 @@
   gcc_assert (TREE_CODE (*expr_p) == MODIFY_EXPR
 	      || TREE_CODE (*expr_p) == INIT_EXPR);
 
+  if (want_value && TREE_THIS_VOLATILE (TREE_TYPE (*to_p)))
+    {
+      /* copy the rhs to a temporary, store the temporary and then
+         return it.  This makes sure we consistently do not re-read a
+         volatile just after storing it.  */
+      tree lhs = *to_p;
+      tree rhs = *from_p;
+      tree nv_type = TYPE_MAIN_VARIANT (TREE_TYPE (lhs));
+      tree tmp = create_tmp_var (nv_type, "modtmp");
+      tree tmp_ass = build2 (MODIFY_EXPR, nv_type, tmp, rhs);
+      tree vol_ass = build2 (MODIFY_EXPR, nv_type, lhs, tmp);
+      
+      recalculate_side_effects (tmp_ass);
+      gimplify_and_add (tmp_ass, pre_p);
+
+      recalculate_side_effects (vol_ass);
+      gimplify_and_add (vol_ass, pre_p);
+
+      *expr_p = tmp;
+      return GS_OK;
+    }
+  
   /* Insert pointer conversions required by the middle-end that are not
      required by the frontend.  This fixes middle-end type checking for
      for example gcc.dg/redecl-6.c.  */
Index: testsuite/gcc.target/i386/volatile-2.c
===================================================================
--- testsuite/gcc.target/i386/volatile-2.c	(revision 0)
+++ testsuite/gcc.target/i386/volatile-2.c	(revision 0)
@@ -0,0 +1,92 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+/* Check volatiles are written, read or not re-read consistently */
+
+
+/* simple assignments */
+
+extern int volatile obj_0;
+void test_0 (int data)
+{
+  /* should not reread obj */
+  /* { dg-final { scan-assembler "movl\[ \t\]\[^,\]+, obj_0" } } */
+  /* { dg-final { scan-assembler-not "movl\[ \t\]obj_0," } } */
+  obj_0 = data;
+}
+
+extern int volatile obj_1;
+int test_1 (int data)
+{
+  /* should not reread obj */
+  /* { dg-final { scan-assembler "movl\[ \t\]\[^,\]+, obj_1" } } */
+  /* { dg-final { scan-assembler-not "movl\[ \t\]obj_1," } } */
+  return obj_1 = data;
+}
+
+extern int volatile obj_2;
+int test_2 (void)
+{
+  /* should not reread obj */
+  /* { dg-final { scan-assembler "movl\[ \t\]\[^,\]+, obj_2" } } */
+  /* { dg-final { scan-assembler-not "movl\[ \t\]obj_2," } } */
+  return obj_2 = 0;
+}
+
+
+/* Assignments in compound exprs */
+
+extern int volatile obj_3;
+int test_3 (int data)
+{
+  /* should not reread obj */
+  /* { dg-final { scan-assembler "movl\[ \t\]\[^,\]+, obj_3" } } */
+  /* { dg-final { scan-assembler-not "movl\[ \t\]obj_3," } } */
+  return (obj_3 = data, 0);
+}
+
+extern int volatile obj_4;
+int test_4 (void)
+{
+  /* should not reread obj */
+  /* { dg-final { scan-assembler "movl\[ \t\]\[^,\]+, obj_4" } } */
+  /* { dg-final { scan-assembler-not "movl\[ \t\]obj_4," } } */
+  return (obj_4 = 0, 0);
+}
+extern int volatile obj_5;
+int test_5 (void)
+{
+  /* should reread obj */
+  /* { dg-final { scan-assembler "movl\[ \t\]\[^,\]+, obj_5" } } */
+  /* { dg-final { scan-assembler "movl\[ \t\]obj_5," } } */
+  return (obj_5 = 0, obj_5);
+}
+
+/* Assignments in conditional exprs */
+
+extern int volatile obj_6;
+void test_6 (int data, int cond)
+{
+  /* should not reread obj */
+  /* { dg-final { scan-assembler "movl\[ \t\]\[^,\]+, obj_6" } } */
+  /* { dg-final { scan-assembler-not "movl\[ \t\]obj_6," } } */
+  cond ? obj_6 = data : 0;
+}
+
+extern int volatile obj_7;
+int test_7 (int data, int cond)
+{
+  /* should not reread obj */
+  /* { dg-final { scan-assembler "movl\[ \t\]\[^,\]+, obj_7" } } */
+  /* { dg-final { scan-assembler-not "movl\[ \t\]obj_7," } } */
+  return cond ? obj_7 = data : 0;
+}
+
+extern int volatile obj_8;
+int test_8 (int cond)
+{
+  /* should not reread obj */
+  /* { dg-final { scan-assembler "movl\[ \t\]\[^,\]+, obj_8" } } */
+  /* { dg-final { scan-assembler-not "movl\[ \t\]obj_8," } } */
+  return cond ? obj_8 = 0 : 0;
+}