Re: [PATCH] D22557: [libcxx] Diagnose invalid memory order arguments in . Fixes PR21179.

[JF Bastien via cfe-commits]

On Jul 22, 2016 4:45 PM, "Eric Fiselier"  wrote:
>
> EricWF added inline comments.
>
> 
> Comment at: test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp:87
> @@ +86,3 @@
> +x.compare_exchange_weak(val1, val2, std::memory_order_release);
> +}
> +{
> 
> jfb wrote:
> > That's not quite true: the failure ordering is auto-deduced from the
success one, but it's not necessarily the same! The spec says all success
is valid, so the auto-mapping has to ensure that all failures are also
valid. That's what I'm trying to have you test: that the auto-mapping is
always valid as well.
> Right, but the auto-mapping is done once we have entered the
`compare_exchange_weak` function, These diagnostics are triggered within
the function signature. So even if we got the auto mapping wrong this test
would not be able to diagnose it.
>
> I agree we should be testing the auto-mapping, but I don't think this
test is the right place to do it.

Ah ok, that's good with me then.

>
> https://reviews.llvm.org/D22557
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22557: [libcxx] Diagnose invalid memory order arguments in . Fixes PR21179.

[Eric Fiselier via cfe-commits]

EricWF added inline comments.


Comment at: test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp:87
@@ +86,3 @@
+x.compare_exchange_weak(val1, val2, std::memory_order_release);
+}
+{

jfb wrote:
> That's not quite true: the failure ordering is auto-deduced from the success 
> one, but it's not necessarily the same! The spec says all success is valid, 
> so the auto-mapping has to ensure that all failures are also valid. That's 
> what I'm trying to have you test: that the auto-mapping is always valid as 
> well.
Right, but the auto-mapping is done once we have entered the 
`compare_exchange_weak` function, These diagnostics are triggered within the 
function signature. So even if we got the auto mapping wrong this test would 
not be able to diagnose it.

I agree we should be testing the auto-mapping, but I don't think this test is 
the right place to do it.


https://reviews.llvm.org/D22557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22557: [libcxx] Diagnose invalid memory order arguments in . Fixes PR21179.

[JF Bastien via cfe-commits]

jfb added inline comments.


Comment at: test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp:87
@@ +86,3 @@
+x.compare_exchange_weak(val1, val2, std::memory_order_release);
+}
+{

That's not quite true: the failure ordering is auto-deduced from the success 
one, but it's not necessarily the same! The spec says all success is valid, so 
the auto-mapping has to ensure that all failures are also valid. That's what 
I'm trying to have you test: that the auto-mapping is always valid as well.


https://reviews.llvm.org/D22557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22557: [libcxx] Diagnose invalid memory order arguments in . Fixes PR21179.

[Eric Fiselier via cfe-commits]

EricWF updated this revision to Diff 65140.
EricWF added a comment.

Add comment about checking "stronger" orderings.


https://reviews.llvm.org/D22557

Files:
  include/atomic
  test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp

Index: test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp
===
--- /dev/null
+++ test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp
@@ -0,0 +1,125 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// The attributes used to diagnose incorrect memory order inputs is clang
+// specific.
+// UNSUPPORTED: gcc
+
+// 
+
+// Test that invalid memory order arguments are diagnosed where possible.
+
+#include 
+
+int main() {
+std::atomic x(42);
+volatile std::atomic& vx = x;
+int val1 = 1;
+int val2 = 2;
+// load operations
+{
+x.load(std::memory_order_release); // expected-error {{operation is invalid}}
+x.load(std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+vx.load(std::memory_order_release); // expected-error {{operation is invalid}}
+vx.load(std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+// valid memory orders
+x.load(std::memory_order_relaxed);
+x.load(std::memory_order_consume);
+x.load(std::memory_order_acquire);
+x.load(std::memory_order_seq_cst);
+}
+{
+std::atomic_load_explicit(, std::memory_order_release); // expected-error {{operation is invalid}}
+std::atomic_load_explicit(, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+std::atomic_load_explicit(, std::memory_order_release); // expected-error {{operation is invalid}}
+std::atomic_load_explicit(, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+// valid memory orders
+std::atomic_load_explicit(, std::memory_order_relaxed);
+std::atomic_load_explicit(, std::memory_order_consume);
+std::atomic_load_explicit(, std::memory_order_acquire);
+std::atomic_load_explicit(, std::memory_order_seq_cst);
+}
+// store operations
+{
+x.store(42, std::memory_order_consume); // expected-error {{operation is invalid}}
+x.store(42, std::memory_order_acquire); // expected-error {{operation is invalid}}
+x.store(42, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+vx.store(42, std::memory_order_consume); // expected-error {{operation is invalid}}
+vx.store(42, std::memory_order_acquire); // expected-error {{operation is invalid}}
+vx.store(42, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+// valid memory orders
+x.store(42, std::memory_order_relaxed);
+x.store(42, std::memory_order_release);
+x.store(42, std::memory_order_seq_cst);
+}
+{
+std::atomic_store_explicit(, 42, std::memory_order_consume); // expected-error {{operation is invalid}}
+std::atomic_store_explicit(, 42, std::memory_order_acquire); // expected-error {{operation is invalid}}
+std::atomic_store_explicit(, 42, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+std::atomic_store_explicit(, 42, std::memory_order_consume); // expected-error {{operation is invalid}}
+std::atomic_store_explicit(, 42, std::memory_order_acquire); // expected-error {{operation is invalid}}
+std::atomic_store_explicit(, 42, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+// valid memory orders
+std::atomic_store_explicit(, 42, std::memory_order_relaxed);
+std::atomic_store_explicit(, 42, std::memory_order_release);
+std::atomic_store_explicit(, 42, std::memory_order_seq_cst);
+}
+// compare exchange weak
+{
+x.compare_exchange_weak(val1, val2, std::memory_order_seq_cst, std::memory_order_release); // expected-error {{operation is invalid}}
+x.compare_exchange_weak(val1, val2, std::memory_order_seq_cst, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+vx.compare_exchange_weak(val1, val2, std::memory_order_seq_cst, std::memory_order_release); // expected-error {{operation is invalid}}
+vx.compare_exchange_weak(val1, val2, std::memory_order_seq_cst, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+// valid memory orders
+x.compare_exchange_weak(val1, val2, std::memory_order_seq_cst, std::memory_order_relaxed);
+x.compare_exchange_weak(val1, val2, 

Re: [PATCH] D22557: [libcxx] Diagnose invalid memory order arguments in . Fixes PR21179.

[Eric Fiselier via cfe-commits]

EricWF added a reviewer: jfb.
EricWF marked an inline comment as done.
EricWF added a comment.

Address inline comments from @jfb and add him as a reviewer.



Comment at: test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp:86
@@ +85,3 @@
+// does not generate any diagnostics.
+x.compare_exchange_weak(val1, val2, std::memory_order_release);
+}

jfb wrote:
> Could you do the other 5 (same below).
I could, but we only check the two argument version of the overload, so I 
*know* the other orderings will have the same result. I think the one test is 
sufficient because it will fail the second we start testing the one argument 
version.

Do you still want the additional tests?


https://reviews.llvm.org/D22557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22557: [libcxx] Diagnose invalid memory order arguments in . Fixes PR21179.

[JF Bastien via cfe-commits]

jfb added a comment.

Two comments, then lgtm.



Comment at: include/atomic:581
@@ +580,3 @@
+   || __f == memory_order_acq_rel, "")))  \
+__attribute__ ((__unavailable__("memory order argument to atomic operation 
is invalid")))
+#endif

OK that's totally fine with me. Could you put this in a comment? Something like 
"not checking the 'stronger' requirement, see wg21.link/p0418" (it'll be 
published in the pre-Issaquah meeting, URL will work then).


Comment at: test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp:86
@@ +85,3 @@
+// does not generate any diagnostics.
+x.compare_exchange_weak(val1, val2, std::memory_order_release);
+}

Could you do the other 5 (same below).


https://reviews.llvm.org/D22557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22557: [libcxx] Diagnose invalid memory order arguments in . Fixes PR21179.

[Ben Craig via cfe-commits]

bcraig added inline comments.


Comment at: include/atomic:569
@@ +568,3 @@
+__attribute__ ((__enable_if__(__m == memory_order_release \
+   || __m == memory_order_acq_rel, "")))  \
+__attribute__ ((__unavailable__("memory order argument to atomic operation 
is invalid")))

Gah, read the logic backwards... and even backwards my comments are incomplete.


https://reviews.llvm.org/D22557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22557: [libcxx] Diagnose invalid memory order arguments in . Fixes PR21179.

[Eric Fiselier via cfe-commits]

EricWF marked 2 inline comments as done.


Comment at: include/atomic:581
@@ +580,3 @@
+   || __f == memory_order_acq_rel, "")))  \
+__attribute__ ((__unavailable__("memory order argument to atomic operation 
is invalid")))
+#endif

jfb wrote:
> This isn't checking for the following requirement from the standard:
> 
> > The failure argument shall be no stronger than the success argument.
> 
> I think that's OK because I intend to remove that requirement from C++ :)
> 
> Should we nonetheless enforce the requirement until the standard drops it? If 
> so, "stronger" isn't well defined by the standard, details here: 
> https://github.com/jfbastien/papers/blob/master/source/D0418r1.bs
Because "stronger" lacks a solid definition I choose not to implement checks 
for that requirement. I believe Clang does as well.

> I think that's OK because I intend to remove that requirement from C++ :)

Yet another reason to not implement it.


https://reviews.llvm.org/D22557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22557: [libcxx] Diagnose invalid memory order arguments in . Fixes PR21179.

[JF Bastien via cfe-commits]

jfb added inline comments.


Comment at: include/atomic:569
@@ +568,3 @@
+__attribute__ ((__enable_if__(__m == memory_order_release \
+   || __m == memory_order_acq_rel, "")))  \
+__attribute__ ((__unavailable__("memory order argument to atomic operation 
is invalid")))

bcraig wrote:
> what about relaxed?
Load relaxed is valid.


https://reviews.llvm.org/D22557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22557: [libcxx] Diagnose invalid memory order arguments in . Fixes PR21179.

[Ben Craig via cfe-commits]

bcraig added a subscriber: bcraig.


Comment at: include/atomic:569
@@ +568,3 @@
+__attribute__ ((__enable_if__(__m == memory_order_release \
+   || __m == memory_order_acq_rel, "")))  \
+__attribute__ ((__unavailable__("memory order argument to atomic operation 
is invalid")))

what about relaxed?


https://reviews.llvm.org/D22557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22557: [libcxx] Diagnose invalid memory order arguments in . Fixes PR21179.

[JF Bastien via cfe-commits]

jfb added a subscriber: jfb.
jfb added a comment.

Awesome, thanks for doing this!

Should this be a warning or an error?



Comment at: include/atomic:581
@@ +580,3 @@
+   || __f == memory_order_acq_rel, "")))  \
+__attribute__ ((__unavailable__("memory order argument to atomic operation 
is invalid")))
+#endif

This isn't checking for the following requirement from the standard:

> The failure argument shall be no stronger than the success argument.

I think that's OK because I intend to remove that requirement from C++ :)

Should we nonetheless enforce the requirement until the standard drops it? If 
so, "stronger" isn't well defined by the standard, details here: 
https://github.com/jfbastien/papers/blob/master/source/D0418r1.bs


Comment at: test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp:30
@@ +29,3 @@
+vx.load(std::memory_order_release); // expected-error {{operation is 
invalid}}
+vx.load(std::memory_order_acq_rel); // expected-error {{operation is 
invalid}}
+}

Could you test that the other memory orders don't have any diagnostic (here and 
below).


Comment at: test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp:55
@@ +54,3 @@
+}
+// compare exchange weak
+{

For cmpxchg (weak and strong), should you also test the version with only a 
`success` ordering? The `failure` one is auto-generated from `success`, but it 
would be good to know that it never generates a diagnostic.


https://reviews.llvm.org/D22557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D22557: [libcxx] Diagnose invalid memory order arguments in . Fixes PR21179.

[Eric Fiselier via cfe-commits]

EricWF created this revision.
EricWF added reviewers: mclow.lists, rsmith.
EricWF added subscribers: cfe-commits, rsmith.

This patch uses the __attribute__((enable_if)) hack suggested by @rsmith to 
diagnose invalid arguments when possible.

In order to diagnose an invalid argument `m` to `f(m)` we provide an additional 
overload of `f` that is only enabled when `m` is invalid. When that function is 
enabled it uses __attribute__((unavailable)) to produce a diagnostic message.

https://reviews.llvm.org/D22557

Files:
  include/atomic
  test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp

Index: test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp
===
--- /dev/null
+++ test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp
@@ -0,0 +1,81 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// The attributes used to diagnose incorrect memory order inputs is clang
+// specific.
+// UNSUPPORTED: gcc
+
+// 
+
+// Test that invalid memory order arguments are diagnosed where possible.
+
+#include 
+
+int main() {
+std::atomic x(42);
+volatile std::atomic& vx = x;
+int val1 = 1;
+int val2 = 2;
+// load operations
+{
+x.load(std::memory_order_release); // expected-error {{operation is invalid}}
+x.load(std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+vx.load(std::memory_order_release); // expected-error {{operation is invalid}}
+vx.load(std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+}
+{
+std::atomic_load_explicit(, std::memory_order_release); // expected-error {{operation is invalid}}
+std::atomic_load_explicit(, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+std::atomic_load_explicit(, std::memory_order_release); // expected-error {{operation is invalid}}
+std::atomic_load_explicit(, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+}
+// store operations
+{
+x.store(42, std::memory_order_consume); // expected-error {{operation is invalid}}
+x.store(42, std::memory_order_acquire); // expected-error {{operation is invalid}}
+x.store(42, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+vx.store(42, std::memory_order_consume); // expected-error {{operation is invalid}}
+vx.store(42, std::memory_order_acquire); // expected-error {{operation is invalid}}
+vx.store(42, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+}
+{
+std::atomic_store_explicit(, 42, std::memory_order_consume); // expected-error {{operation is invalid}}
+std::atomic_store_explicit(, 42, std::memory_order_acquire); // expected-error {{operation is invalid}}
+std::atomic_store_explicit(, 42, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+std::atomic_store_explicit(, 42, std::memory_order_consume); // expected-error {{operation is invalid}}
+std::atomic_store_explicit(, 42, std::memory_order_acquire); // expected-error {{operation is invalid}}
+std::atomic_store_explicit(, 42, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+}
+// compare exchange weak
+{
+x.compare_exchange_weak(val1, val2, std::memory_order_seq_cst, std::memory_order_release); // expected-error {{operation is invalid}}
+x.compare_exchange_weak(val1, val2, std::memory_order_seq_cst, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+vx.compare_exchange_weak(val1, val2, std::memory_order_seq_cst, std::memory_order_release); // expected-error {{operation is invalid}}
+vx.compare_exchange_weak(val1, val2, std::memory_order_seq_cst, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+}
+{
+std::atomic_compare_exchange_weak_explicit(, , val2, std::memory_order_seq_cst, std::memory_order_release); // expected-error {{operation is invalid}}
+std::atomic_compare_exchange_weak_explicit(, , val2, std::memory_order_seq_cst, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+std::atomic_compare_exchange_weak_explicit(, , val2, std::memory_order_seq_cst, std::memory_order_release); // expected-error {{operation is invalid}}
+std::atomic_compare_exchange_weak_explicit(, , val2, std::memory_order_seq_cst, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+}
+// compare exchange strong
+{
+x.compare_exchange_strong(val1, val2,