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
