[PATCH 1/2] x86,s390: add compiler memory barriers when expanding atomic_thread_fence (PR 80640)

[Alexander Monakov <amonakov at ispras dot ru>]

Hi,

When expanding __atomic_thread_fence(x) to RTL, the i386 backend doesn't emit
any instruction except for x==__ATOMIC_SEQ_CST (which emits 'mfence').  This 
is incorrect: although no machine barrier is needed, the compiler still must
emit a compiler barrier into the IR to prevent propagation and code motion
across the fence.  The testcase added with the patch shows how it can lead
to a miscompilation.

The proposed patch fixes it by handling non-seq-cst fences exactly like
__atomic_signal_fence is expanded, by emitting asm volatile("":::"memory").

The s390 backend uses the a similar mem_thread_fence expansion, so the patch
fixes both backends in the same manner.

Bootstrapped and regtested on x86_64; also checked that s390-linux cc1
successfully builds after the change.  OK for trunk?

(the original source code in the PR was misusing atomic fences by doing
something like

  void f(int *p)
  {
    while (*p)
      __atomic_thread_fence(__ATOMIC_ACQUIRE);
  }

but since *p is not atomic, a concurrent write to *p would cause a data race and
thus invoke undefined behavior; also, if *p is false prior to entering the loop,
execution does not encounter the fence; new test here has code usable without UB)

Alexander

	* config/i386/sync.md (mem_thread_fence): Emit a compiler barrier for
	non-seq-cst fences.  Adjust comment.
	* config/s390/s390.md (mem_thread_fence): Likewise.
	* optabs.c (expand_asm_memory_barrier): Export.
	* optabs.h (expand_asm_memory_barrier): Declare.
testsuite/
	* gcc.target/i386/pr80640-1.c: New testcase.
---
 gcc/config/i386/sync.md                   |  7 ++++++-
 gcc/config/s390/s390.md                   | 11 +++++++++--
 gcc/optabs.c                              |  2 +-
 gcc/optabs.h                              |  3 +++
 gcc/testsuite/gcc.target/i386/pr80640-1.c | 12 ++++++++++++
 5 files changed, 31 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr80640-1.c

diff --git a/gcc/config/i386/sync.md b/gcc/config/i386/sync.md
index 20d46fe..619d53b 100644
--- a/gcc/config/i386/sync.md
+++ b/gcc/config/i386/sync.md
@@ -108,7 +108,7 @@ (define_expand "mem_thread_fence"
   enum memmodel model = memmodel_from_int (INTVAL (operands[0]));
 
   /* Unless this is a SEQ_CST fence, the i386 memory model is strong
-     enough not to require barriers of any kind.  */
+     enough not to require a processor barrier of any kind.  */
   if (is_mm_seq_cst (model))
     {
       rtx (*mfence_insn)(rtx);
@@ -124,6 +124,11 @@ (define_expand "mem_thread_fence"
 
       emit_insn (mfence_insn (mem));
     }
+  else if (!is_mm_relaxed (model))
+    {
+      /* However, a compiler barrier is still required.  */
+      expand_asm_memory_barrier ();
+    }
   DONE;
 })
 
diff --git a/gcc/config/s390/s390.md b/gcc/config/s390/s390.md
index c9fd19a..65e54c4 100644
--- a/gcc/config/s390/s390.md
+++ b/gcc/config/s390/s390.md
@@ -10109,14 +10109,21 @@ (define_expand "mem_thread_fence"
   [(match_operand:SI 0 "const_int_operand")]		;; model
   ""
 {
+  enum memmodel model = memmodel_from_int (INTVAL (operands[0]));
+
   /* Unless this is a SEQ_CST fence, the s390 memory model is strong
-     enough not to require barriers of any kind.  */
-  if (is_mm_seq_cst (memmodel_from_int (INTVAL (operands[0]))))
+     enough not to require a processor barrier of any kind.  */
+  if (is_mm_seq_cst (model))
     {
       rtx mem = gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (Pmode));
       MEM_VOLATILE_P (mem) = 1;
       emit_insn (gen_mem_thread_fence_1 (mem));
     }
+  else if (!is_mm_relaxed (model))
+    {
+      /* However, a compiler barrier is still required.  */
+      expand_asm_memory_barrier ();
+    }
   DONE;
 })
 
diff --git a/gcc/optabs.c b/gcc/optabs.c
index 48e37f8..1f1fbc3 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -6269,7 +6269,7 @@ expand_atomic_compare_and_swap (rtx *ptarget_bool, rtx *ptarget_oval,
 
 /* Generate asm volatile("" : : : "memory") as the memory barrier.  */
 
-static void
+void
 expand_asm_memory_barrier (void)
 {
   rtx asm_op, clob;
diff --git a/gcc/optabs.h b/gcc/optabs.h
index b2dd31a..aca6755 100644
--- a/gcc/optabs.h
+++ b/gcc/optabs.h
@@ -322,6 +322,9 @@ extern bool expand_atomic_compare_and_swap (rtx *, rtx *, rtx, rtx, rtx, bool,
 extern void expand_mem_thread_fence (enum memmodel);
 extern void expand_mem_signal_fence (enum memmodel);
 
+/* Generate a compile-time memory barrier.  */
+extern void expand_asm_memory_barrier (void);
+
 rtx expand_atomic_load (rtx, rtx, enum memmodel);
 rtx expand_atomic_store (rtx, rtx, enum memmodel, bool);
 rtx expand_atomic_fetch_op (rtx, rtx, rtx, enum rtx_code, enum memmodel, 
diff --git a/gcc/testsuite/gcc.target/i386/pr80640-1.c b/gcc/testsuite/gcc.target/i386/pr80640-1.c
new file mode 100644
index 0000000..f1d1e55
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr80640-1.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-tree-ter" } */
+/* { dg-final { scan-assembler-not "xorl\[\\t \]*\\\%eax,\[\\t \]*%eax" } } */
+
+int f(int *p, volatile _Bool *p1, volatile _Bool *p2)
+{
+  int v = *p;
+  *p1 = !v;
+  while (*p2);
+  __atomic_thread_fence (__ATOMIC_ACQUIRE);
+  return v - *p;
+}
-- 
1.8.3.1