From: Alexander Monakov <amonakov@ispras.ru>
To: gcc-patches@gcc.gnu.org
Subject: [PATCH 1/2] x86,s390: add compiler memory barriers when expanding atomic_thread_fence (PR 80640)
Date: Wed, 10 May 2017 14:06:00 -0000 [thread overview]
Message-ID: <alpine.LNX.2.20.13.1705101620050.11444@monopod.intra.ispras.ru> (raw)
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
next reply other threads:[~2017-05-10 14:01 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-10 14:06 Alexander Monakov [this message]
2017-05-10 14:21 ` Alexander Monakov
2017-05-17 9:57 ` Alexander Monakov
2017-05-26 11:05 ` Alexander Monakov
2017-06-07 21:55 ` Alexander Monakov
2017-07-11 18:39 ` Alexander Monakov
2017-07-26 17:08 ` Jeff Law
2017-07-26 17:19 ` Alexander Monakov
2017-07-26 17:35 ` Jeff Law
2017-07-26 18:13 ` Alexander Monakov
2017-07-26 20:02 ` Alexander Monakov
2017-07-31 15:48 ` Jeff Law
2017-07-31 15:50 ` Jeff Law
2017-07-31 17:02 ` Alexander Monakov
2017-07-31 17:32 ` Jeff Law
2017-07-31 18:01 ` Alexander Monakov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.LNX.2.20.13.1705101620050.11444@monopod.intra.ispras.ru \
--to=amonakov@ispras.ru \
--cc=gcc-patches@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).