Re: [PATCH] D22557: [libcxx] Diagnose invalid memory order arguments in . Fixes PR21179.
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.
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.
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.
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(&x, std::memory_order_release); // expected-error {{operation is invalid}} +std::atomic_load_explicit(&x, std::memory_order_acq_rel); // expected-error {{operation is invalid}} +std::atomic_load_explicit(&vx, std::memory_order_release); // expected-error {{operation is invalid}} +std::atomic_load_explicit(&vx, std::memory_order_acq_rel); // expected-error {{operation is invalid}} +// valid memory orders +std::atomic_load_explicit(&x, std::memory_order_relaxed); +std::atomic_load_explicit(&x, std::memory_order_consume); +std::atomic_load_explicit(&x, std::memory_order_acquire); +std::atomic_load_explicit(&x, 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(&x, 42, std::memory_order_consume); // expected-error {{operation is invalid}} +std::atomic_store_explicit(&x, 42, std::memory_order_acquire); // expected-error {{operation is invalid}} +std::atomic_store_explicit(&x, 42, std::memory_order_acq_rel); // expected-error {{operation is invalid}} +std::atomic_store_explicit(&vx, 42, std::memory_order_consume); // expected-error {{operation is invalid}} +std::atomic_store_explicit(&vx, 42, std::memory_order_acquire); // expected-error {{operation is invalid}} +std::atomic_store_explicit(&vx, 42, std::memory_order_acq_rel); // expected-error {{operation is invalid}} +// valid memory orders +std::atomic_store_explicit(&x, 42, std::memory_order_relaxed); +std::atomic_store_explicit(&x, 42, std::memory_order_release); +std::atomic_store_explicit(&x, 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_exc
Re: [PATCH] D22557: [libcxx] Diagnose invalid memory order arguments in . Fixes PR21179.
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.
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.
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.
EricWF updated this revision to Diff 64808. EricWF added a comment. Add tests for valid memory orderings as well. Originally I figured the rest of the test suite should cover them but there's no harm in more tests. 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(&x, std::memory_order_release); // expected-error {{operation is invalid}} +std::atomic_load_explicit(&x, std::memory_order_acq_rel); // expected-error {{operation is invalid}} +std::atomic_load_explicit(&vx, std::memory_order_release); // expected-error {{operation is invalid}} +std::atomic_load_explicit(&vx, std::memory_order_acq_rel); // expected-error {{operation is invalid}} +// valid memory orders +std::atomic_load_explicit(&x, std::memory_order_relaxed); +std::atomic_load_explicit(&x, std::memory_order_consume); +std::atomic_load_explicit(&x, std::memory_order_acquire); +std::atomic_load_explicit(&x, 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(&x, 42, std::memory_order_consume); // expected-error {{operation is invalid}} +std::atomic_store_explicit(&x, 42, std::memory_order_acquire); // expected-error {{operation is invalid}} +std::atomic_store_explicit(&x, 42, std::memory_order_acq_rel); // expected-error {{operation is invalid}} +std::atomic_store_explicit(&vx, 42, std::memory_order_consume); // expected-error {{operation is invalid}} +std::atomic_store_explicit(&vx, 42, std::memory_order_acquire); // expected-error {{operation is invalid}} +std::atomic_store_explicit(&vx, 42, std::memory_order_acq_rel); // expected-error {{operation is invalid}} +// valid memory orders +std::atomic_store_explicit(&x, 42, std::memory_order_relaxed); +std::atomic_store_explicit(&x, 42, std::memory_order_release); +std::atomic_store_explicit(&x, 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_exch
Re: [PATCH] D22557: [libcxx] Diagnose invalid memory order arguments in . Fixes PR21179.
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.
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.
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.
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.
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(&x, std::memory_order_release); // expected-error {{operation is invalid}} +std::atomic_load_explicit(&x, std::memory_order_acq_rel); // expected-error {{operation is invalid}} +std::atomic_load_explicit(&vx, std::memory_order_release); // expected-error {{operation is invalid}} +std::atomic_load_explicit(&vx, 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(&x, 42, std::memory_order_consume); // expected-error {{operation is invalid}} +std::atomic_store_explicit(&x, 42, std::memory_order_acquire); // expected-error {{operation is invalid}} +std::atomic_store_explicit(&x, 42, std::memory_order_acq_rel); // expected-error {{operation is invalid}} +std::atomic_store_explicit(&vx, 42, std::memory_order_consume); // expected-error {{operation is invalid}} +std::atomic_store_explicit(&vx, 42, std::memory_order_acquire); // expected-error {{operation is invalid}} +std::atomic_store_explicit(&vx, 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(&x, &val1, val2, std::memory_order_seq_cst, std::memory_order_release); // expected-error {{operation is invalid}} +std::atomic_compare_exchange_weak_explicit(&x, &val1, val2, std::memory_order_seq_cst, std::memory_order_acq_rel); // expected-error {{operation is invalid}} +std::atomic_compare_exchange_weak_explicit(&vx, &val1, val2, std::memory_order_seq_cst, std::memory_order_release); // expected-error {{operation is invalid}} +std::atomic_compare_exchange_weak_explicit(&vx, &val1, val2, std::memory_order_seq_cst, std::memory_order_acq_rel); // expected-error {{operation is invalid}} +} +// compare exchange strong +