Patchwork PR54149: fix data race in LIM pass

login
register
mail settings
Submitter Aldy Hernandez
Date Sept. 10, 2012, 11:15 p.m.
Message ID <504E7476.9070103@redhat.com>
Download mbox | patch
Permalink /patch/182969/
State New
Headers show

Comments

Aldy Hernandez - Sept. 10, 2012, 11:15 p.m.
In this failing testcase the LIM pass writes to g_13 regardless of the 
initial value of g_13, which is the test protecting the write.  This 
causes an incorrect store data race wrt both the C++ memory model and 
transactional memory (the latter if the store occurs inside of a 
transaction).

The problem here is that the ``lsm_flag'' temporary should only be set 
to true on the code paths where we actually set the original global.  As 
it stands, we are setting lsm_flag to true for reads or writes.

Fixed by only setting lsm_flag=1 when the original code path has a write.

Tested on x86-64 Linux.

OK for trunk?
PR middle-end/54149
	* tree-ssa-loop-im.c (execute_sm_if_changed_flag_set): Only set
	flag for writes.
Richard Guenther - Sept. 11, 2012, 8:56 a.m.
On Tue, Sep 11, 2012 at 1:15 AM, Aldy Hernandez <aldyh@redhat.com> wrote:
> In this failing testcase the LIM pass writes to g_13 regardless of the
> initial value of g_13, which is the test protecting the write.  This causes
> an incorrect store data race wrt both the C++ memory model and transactional
> memory (the latter if the store occurs inside of a transaction).
>
> The problem here is that the ``lsm_flag'' temporary should only be set to
> true on the code paths where we actually set the original global.  As it
> stands, we are setting lsm_flag to true for reads or writes.
>
> Fixed by only setting lsm_flag=1 when the original code path has a write.
>
> Tested on x86-64 Linux.
>
> OK for trunk?

+      /* Only set the flag for writes.  */
+      if (is_gimple_assign (loc->stmt)
+	  && gimple_assign_lhs (loc->stmt) == *loc->ref)

ok with

  && gimple_assign_lhs_ptr (loc->stmt) == loc->ref

instead.  Let's hope we conservatively catch all writes to ref this way (which
is what we need, right)?

Thanks,
Richard.

Patch

diff --git a/gcc/testsuite/gcc.dg/simulate-thread/speculative-store-4.c b/gcc/testsuite/gcc.dg/simulate-thread/speculative-store-4.c
new file mode 100644
index 0000000..735cf5b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/simulate-thread/speculative-store-4.c
@@ -0,0 +1,54 @@ 
+/* { dg-do link } */
+/* { dg-options "--param allow-store-data-races=0 -O" } */
+/* { dg-final { simulate-thread } } */
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "simulate-thread.h"
+
+/* PR 54139 */
+/* Test that speculative stores do not happen for --param
+   allow-store-data-races=0.  */
+
+int g_13=1, insns=1;
+
+__attribute__((noinline))
+void simulate_thread_main()
+{
+  int l_245;
+
+  /* Since g_13 is unilaterally set positive above, there should be
+     no store to g_13 below.  */
+  for (l_245 = 0; l_245 <= 1; l_245 += 1)
+    for (; g_13 <= 0; g_13 = 1)
+      ;
+}
+
+int main()
+{
+  simulate_thread_main ();
+  simulate_thread_done ();
+  return 0;
+}
+
+void simulate_thread_other_threads ()
+{
+  ++g_13;
+  ++insns;
+}
+
+int simulate_thread_step_verify ()
+{
+  return 0;
+}
+
+int simulate_thread_final_verify ()
+{
+  if (g_13 != insns)
+    {
+      printf("FAIL: g_13 was incorrectly cached\n");
+      return 1;
+    }
+  return 0;
+}
diff --git a/gcc/tree-ssa-loop-im.c b/gcc/tree-ssa-loop-im.c
index 0f61631..c5fc324 100644
--- a/gcc/tree-ssa-loop-im.c
+++ b/gcc/tree-ssa-loop-im.c
@@ -2113,9 +2113,14 @@  execute_sm_if_changed_flag_set (struct loop *loop, mem_ref_p ref)
       gimple_stmt_iterator gsi;
       gimple stmt;
 
-      gsi = gsi_for_stmt (loc->stmt);
-      stmt = gimple_build_assign (flag, boolean_true_node);
-      gsi_insert_after (&gsi, stmt, GSI_CONTINUE_LINKING);
+      /* Only set the flag for writes.  */
+      if (is_gimple_assign (loc->stmt)
+	  && gimple_assign_lhs (loc->stmt) == *loc->ref)
+	{
+	  gsi = gsi_for_stmt (loc->stmt);
+	  stmt = gimple_build_assign (flag, boolean_true_node);
+	  gsi_insert_after (&gsi, stmt, GSI_CONTINUE_LINKING);
+	}
     }
   VEC_free (mem_ref_loc_p, heap, locs);
   return flag;