[Sema] atomic_compare_exchange: check failure memory order #74959
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For
`__atomic_compare_exchange{,_n}/__c11_atomic_compare_exchange_{strong,weak}`,
GCC checks both the success memory order and the failure memory order
under the default -Winvalid-memory-model ("memory model" is confusing
here and "memory order" is much more common in the atomic context).
* The failure memory order cannot be stronger than the success memory
order.
* The failure memory order, if a constant, must be one of
relaxed/consume/acquire/seq_cst.
Clang checks just the success memory order under the default
-Watomic-memory-ordering. This patch checks the failure memory order.
MaskRay
requested review from
gandhi56,
yxsamliu,
jhuber6,
cor3ntin and
AaronBallman
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Fangrui Song (MaskRay) ChangesFor
Clang checks just the success memory order under the default Full diff: https://github.com/llvm/llvm-project/pull/74959.diff 5 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index caee2dc6daadb6..a282f37292a037 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -822,6 +822,7 @@ def UndeclaredSelector : DiagGroup<"undeclared-selector">;
def ImplicitAtomic : DiagGroup<"implicit-atomic-properties">;
def AtomicAlignment : DiagGroup<"atomic-alignment">;
def CustomAtomic : DiagGroup<"custom-atomic-properties">;
+def AtomicMemoryOrdering : DiagGroup<"atomic-memory-ordering">;
def AtomicProperties : DiagGroup<"atomic-properties",
[ImplicitAtomic, CustomAtomic]>;
def SyncAlignment : DiagGroup<"sync-alignment">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 28d95ca9b13893..03ed23473c8d32 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8728,7 +8728,13 @@ def err_atomic_op_needs_atomic_int : Error<
"%select{|atomic }0integer (%1 invalid)">;
def warn_atomic_op_has_invalid_memory_order : Warning<
"memory order argument to atomic operation is invalid">,
- InGroup<DiagGroup<"atomic-memory-ordering">>;
+ InGroup<AtomicMemoryOrdering>;
+def warn_atomic_op_has_invalid_failure_memory_order : Warning<
+ "failure memory order argument to atomic operation is invalid">,
+ InGroup<AtomicMemoryOrdering>;
+def warn_atomic_op_failure_memory_order_stronger_than_success : Warning<
+ "failure memory order cannot be stronger than success memory order">,
+ InGroup<AtomicMemoryOrdering>;
def err_atomic_op_has_invalid_synch_scope : Error<
"synchronization scope argument to atomic operation is invalid">;
def warn_atomic_implicit_seq_cst : Warning<
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 5c97346184470a..4b1f2ccd27c01d 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -8181,13 +8181,33 @@ ExprResult Sema::BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
break;
}
+ // If the memory orders are constants, check they are valid.
if (SubExprs.size() >= 2 && Form != Init) {
- if (std::optional<llvm::APSInt> Result =
- SubExprs[1]->getIntegerConstantExpr(Context))
- if (!isValidOrderingForOp(Result->getSExtValue(), Op))
- Diag(SubExprs[1]->getBeginLoc(),
- diag::warn_atomic_op_has_invalid_memory_order)
- << SubExprs[1]->getSourceRange();
+ std::optional<llvm::APSInt> Success =
+ SubExprs[1]->getIntegerConstantExpr(Context);
+ if (Success && !isValidOrderingForOp(Success->getSExtValue(), Op))
+ Diag(SubExprs[1]->getBeginLoc(),
+ diag::warn_atomic_op_has_invalid_memory_order)
+ << SubExprs[1]->getSourceRange();
+ if (SubExprs.size() >= 5) {
+ if (std::optional<llvm::APSInt> Failure =
+ SubExprs[3]->getIntegerConstantExpr(Context)) {
+ if (!llvm::is_contained(
+ {llvm::AtomicOrderingCABI::relaxed,
+ llvm::AtomicOrderingCABI::consume,
+ llvm::AtomicOrderingCABI::acquire,
+ llvm::AtomicOrderingCABI::seq_cst},
+ (llvm::AtomicOrderingCABI)Failure->getSExtValue())) {
+ Diag(SubExprs[3]->getBeginLoc(),
+ diag::warn_atomic_op_has_invalid_failure_memory_order)
+ << SubExprs[3]->getSourceRange();
+ } else if (Success && *Success < *Failure) {
+ Diag(SubExprs[3]->getBeginLoc(),
+ diag::warn_atomic_op_failure_memory_order_stronger_than_success)
+ << SubExprs[3]->getSourceRange();
+ }
+ }
+ }
}
if (auto ScopeModel = AtomicExpr::getScopeModel(Op)) {
diff --git a/clang/test/Sema/atomic-ops.c b/clang/test/Sema/atomic-ops.c
index 4fa1223b3038f3..d8bf366c44a351 100644
--- a/clang/test/Sema/atomic-ops.c
+++ b/clang/test/Sema/atomic-ops.c
@@ -432,14 +432,28 @@ void memory_checks(_Atomic(int) *Ap, int *p, int val) {
(void)__c11_atomic_compare_exchange_strong(Ap, p, val, memory_order_consume, memory_order_relaxed);
(void)__c11_atomic_compare_exchange_strong(Ap, p, val, memory_order_release, memory_order_relaxed);
(void)__c11_atomic_compare_exchange_strong(Ap, p, val, memory_order_acq_rel, memory_order_relaxed);
- (void)__c11_atomic_compare_exchange_strong(Ap, p, val, memory_order_seq_cst, memory_order_relaxed);
+ (void)__c11_atomic_compare_exchange_strong(Ap, p, val, memory_order_seq_cst, memory_order_acquire);
+ (void)__c11_atomic_compare_exchange_strong(Ap, p, val, memory_order_seq_cst, memory_order_consume);
+ (void)__c11_atomic_compare_exchange_strong(Ap, p, val, memory_order_seq_cst, memory_order_release); // expected-warning {{memory order argument to atomic operation is invalid}}
+ (void)__c11_atomic_compare_exchange_strong(Ap, p, val, memory_order_seq_cst, memory_order_acq_rel); // expected-warning {{memory order argument to atomic operation is invalid}}
+ (void)__c11_atomic_compare_exchange_strong(Ap, p, val, memory_order_seq_cst, memory_order_seq_cst);
+ (void)__c11_atomic_compare_exchange_strong(Ap, p, val, memory_order_seq_cst, -1); // expected-warning {{memory order argument to atomic operation is invalid}}
+ (void)__c11_atomic_compare_exchange_strong(Ap, p, val, memory_order_relaxed, memory_order_acquire); // expected-warning {{failure memory order cannot be stronger than success memory order}}
+ (void)__c11_atomic_compare_exchange_strong(Ap, p, val, memory_order_acquire, memory_order_seq_cst); // expected-warning {{failure memory order cannot be stronger than success memory order}}
(void)__c11_atomic_compare_exchange_weak(Ap, p, val, memory_order_relaxed, memory_order_relaxed);
(void)__c11_atomic_compare_exchange_weak(Ap, p, val, memory_order_acquire, memory_order_relaxed);
(void)__c11_atomic_compare_exchange_weak(Ap, p, val, memory_order_consume, memory_order_relaxed);
(void)__c11_atomic_compare_exchange_weak(Ap, p, val, memory_order_release, memory_order_relaxed);
(void)__c11_atomic_compare_exchange_weak(Ap, p, val, memory_order_acq_rel, memory_order_relaxed);
- (void)__c11_atomic_compare_exchange_weak(Ap, p, val, memory_order_seq_cst, memory_order_relaxed);
+ (void)__c11_atomic_compare_exchange_weak(Ap, p, val, memory_order_seq_cst, memory_order_acquire);
+ (void)__c11_atomic_compare_exchange_weak(Ap, p, val, memory_order_seq_cst, memory_order_consume);
+ (void)__c11_atomic_compare_exchange_weak(Ap, p, val, memory_order_seq_cst, memory_order_release); // expected-warning {{memory order argument to atomic operation is invalid}}
+ (void)__c11_atomic_compare_exchange_weak(Ap, p, val, memory_order_seq_cst, memory_order_acq_rel); // expected-warning {{memory order argument to atomic operation is invalid}}
+ (void)__c11_atomic_compare_exchange_weak(Ap, p, val, memory_order_seq_cst, memory_order_seq_cst);
+ (void)__c11_atomic_compare_exchange_weak(Ap, p, val, memory_order_seq_cst, -1); // expected-warning {{memory order argument to atomic operation is invalid}}
+ (void)__c11_atomic_compare_exchange_weak(Ap, p, val, memory_order_relaxed, memory_order_acquire); // expected-warning {{failure memory order cannot be stronger than success memory order}}
+ (void)__c11_atomic_compare_exchange_weak(Ap, p, val, memory_order_acquire, memory_order_seq_cst); // expected-warning {{failure memory order cannot be stronger than success memory order}}
(void)__atomic_load_n(p, memory_order_relaxed);
(void)__atomic_load_n(p, memory_order_acquire);
@@ -600,7 +614,12 @@ void memory_checks(_Atomic(int) *Ap, int *p, int val) {
(void)__atomic_compare_exchange(p, p, p, 0, memory_order_consume, memory_order_relaxed);
(void)__atomic_compare_exchange(p, p, p, 0, memory_order_release, memory_order_relaxed);
(void)__atomic_compare_exchange(p, p, p, 0, memory_order_acq_rel, memory_order_relaxed);
- (void)__atomic_compare_exchange(p, p, p, 0, memory_order_seq_cst, memory_order_relaxed);
+ (void)__atomic_compare_exchange(p, p, p, 0, memory_order_seq_cst, memory_order_acquire);
+ (void)__atomic_compare_exchange(p, p, p, 0, memory_order_seq_cst, memory_order_consume);
+ (void)__atomic_compare_exchange(p, p, p, 0, memory_order_seq_cst, memory_order_release); // expected-warning {{failure memory order argument to atomic operation is invalid}}
+ (void)__atomic_compare_exchange(p, p, p, 0, memory_order_seq_cst, memory_order_acq_rel); // expected-warning {{failure memory order argument to atomic operation is invalid}}
+ (void)__atomic_compare_exchange(p, p, p, 0, memory_order_seq_cst, memory_order_seq_cst);
+ (void)__atomic_compare_exchange(p, p, p, 0, memory_order_seq_cst, -1); // expected-warning {{memory order argument to atomic operation is invalid}}
(void)__atomic_compare_exchange_n(p, p, val, 0, memory_order_relaxed, memory_order_relaxed);
(void)__atomic_compare_exchange_n(p, p, val, 0, memory_order_acquire, memory_order_relaxed);
@@ -608,6 +627,12 @@ void memory_checks(_Atomic(int) *Ap, int *p, int val) {
(void)__atomic_compare_exchange_n(p, p, val, 0, memory_order_release, memory_order_relaxed);
(void)__atomic_compare_exchange_n(p, p, val, 0, memory_order_acq_rel, memory_order_relaxed);
(void)__atomic_compare_exchange_n(p, p, val, 0, memory_order_seq_cst, memory_order_relaxed);
+ (void)__atomic_compare_exchange_n(p, p, val, 0, memory_order_seq_cst, memory_order_acquire);
+ (void)__atomic_compare_exchange_n(p, p, val, 0, memory_order_seq_cst, memory_order_consume);
+ (void)__atomic_compare_exchange_n(p, p, val, 0, memory_order_seq_cst, memory_order_release); // expected-warning {{failure memory order argument to atomic operation is invalid}}
+ (void)__atomic_compare_exchange_n(p, p, val, 0, memory_order_seq_cst, memory_order_acq_rel); // expected-warning {{failure memory order argument to atomic operation is invalid}}
+ (void)__atomic_compare_exchange_n(p, p, val, 0, memory_order_seq_cst, memory_order_seq_cst);
+ (void)__atomic_compare_exchange_n(p, p, val, 0, memory_order_seq_cst, -1); // expected-warning {{memory order argument to atomic operation is invalid}}
}
void nullPointerWarning(void) {
diff --git a/clang/test/SemaCUDA/atomic-ops.cu b/clang/test/SemaCUDA/atomic-ops.cu
index af93b7e1e79448..0b22e81ec9ea3b 100644
--- a/clang/test/SemaCUDA/atomic-ops.cu
+++ b/clang/test/SemaCUDA/atomic-ops.cu
@@ -73,10 +73,10 @@ __device__ bool test_hip_atomic_cmpxchg_weak(int *ptr, int val, int desired) {
flag = __hip_atomic_compare_exchange_weak(ptr, &val, desired, __ATOMIC_RELAXED, __ATOMIC_RELAXED, __HIP_MEMORY_SCOPE_WORKGROUP);
flag = __hip_atomic_compare_exchange_weak(ptr, &val, desired, __ATOMIC_RELAXED, __ATOMIC_RELAXED, __HIP_MEMORY_SCOPE_AGENT);
flag = __hip_atomic_compare_exchange_weak(ptr, &val, desired, __ATOMIC_RELAXED, __ATOMIC_RELAXED, __HIP_MEMORY_SCOPE_SINGLETHREAD);
- flag = __hip_atomic_compare_exchange_weak(ptr, &val, desired, __ATOMIC_RELAXED, __ATOMIC_SEQ_CST, __HIP_MEMORY_SCOPE_SINGLETHREAD);
- flag = __hip_atomic_compare_exchange_weak(ptr, &val, desired, __ATOMIC_RELAXED, __ATOMIC_CONSUME, __HIP_MEMORY_SCOPE_SINGLETHREAD);
- flag = __hip_atomic_compare_exchange_weak(ptr, &val, desired, __ATOMIC_RELAXED, __ATOMIC_ACQUIRE, __HIP_MEMORY_SCOPE_SINGLETHREAD);
- flag = __hip_atomic_compare_exchange_weak(ptr, &val, desired, __ATOMIC_RELAXED, __ATOMIC_ACQ_REL, __HIP_MEMORY_SCOPE_SINGLETHREAD);
+ flag = __hip_atomic_compare_exchange_weak(ptr, &val, desired, __ATOMIC_RELAXED, __ATOMIC_SEQ_CST, __HIP_MEMORY_SCOPE_SINGLETHREAD); // expected-warning {{failure memory order cannot be stronger than success memory order}}
+ flag = __hip_atomic_compare_exchange_weak(ptr, &val, desired, __ATOMIC_RELAXED, __ATOMIC_CONSUME, __HIP_MEMORY_SCOPE_SINGLETHREAD); // expected-warning {{failure memory order cannot be stronger than success memory order}}
+ flag = __hip_atomic_compare_exchange_weak(ptr, &val, desired, __ATOMIC_RELAXED, __ATOMIC_ACQUIRE, __HIP_MEMORY_SCOPE_SINGLETHREAD); // expected-warning {{failure memory order cannot be stronger than success memory order}}
+ flag = __hip_atomic_compare_exchange_weak(ptr, &val, desired, __ATOMIC_RELAXED, __ATOMIC_ACQ_REL, __HIP_MEMORY_SCOPE_SINGLETHREAD); // expected-warning {{failure memory order argument to atomic operation is invalid}}
flag = __hip_atomic_compare_exchange_weak(ptr, &val, desired, __ATOMIC_RELAXED, __ATOMIC_RELAXED, __HIP_MEMORY_SCOPE_SINGLETHREAD);
flag = __hip_atomic_compare_exchange_weak(ptr, &val, desired, __ATOMIC_SEQ_CST, __ATOMIC_RELAXED, __HIP_MEMORY_SCOPE_SINGLETHREAD);
flag = __hip_atomic_compare_exchange_weak(ptr, &val, desired, __ATOMIC_CONSUME, __ATOMIC_RELAXED, __HIP_MEMORY_SCOPE_SINGLETHREAD);
|
jyknight
requested changes
Dec 11, 2023
| @@ -8728,7 +8728,13 @@ def err_atomic_op_needs_atomic_int : Error< | |||
| "%select{|atomic }0integer (%1 invalid)">; | |||
| def warn_atomic_op_has_invalid_memory_order : Warning< | |||
| "memory order argument to atomic operation is invalid">, | |||
| InGroup<DiagGroup<"atomic-memory-ordering">>; | |||
| InGroup<AtomicMemoryOrdering>; | |||
| def warn_atomic_op_has_invalid_failure_memory_order : Warning< | |||
There was a problem hiding this comment.
Merge with former diagnostic by adding:
%select{|success |failure } to the beginning of the message, and adjusting callers to stream 0, 1, or 2 as the first param, for non-cmpxchg, cmpxchg-success, cmpxchg-failure.
Comment on lines
8735
to
8737
| def warn_atomic_op_failure_memory_order_stronger_than_success : Warning< | ||
| "failure memory order cannot be stronger than success memory order">, | ||
| InGroup<AtomicMemoryOrdering>; |
There was a problem hiding this comment.
This isn't a requirement any more, since C++17, and we applied that to all language modes. So remove this diagnostic.
https://eel.is/c++draft/atomics#ref.ops-18
There was a problem hiding this comment.
Removed this warning even if GCC checks it.
compare_exchange_strong precondition does not require it.
|
PR description needs to be updated after changes. Otherwise LGTM. |
Thanks. Updated the description. |
|
@jyknight If this looks good, mind changing the status from "request changes"? :) I'll wait a bit in case others have an opinion. |
|
Did this change anything for the |
I am not familiar with the scoped atomic xchg stuff, but this patch needs to update one |
jyknight
approved these changes
Dec 14, 2023
The scoped variant(s) use the same codepath here, so they'll also verify that the failure order is valid. |
yxsamliu
approved these changes
Dec 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For
__atomic_compare_exchange{,_n}/__c11_atomic_compare_exchange_{strong,weak},GCC checks both the success memory order and the failure memory order
under the default -Winvalid-memory-model ("memory model" is confusing
here and "memory order" is much more common in the atomic context).
relaxed/consume/acquire/seq_cst.
Clang checks just the success memory order under the default
-Watomic-memory-ordering. This patch checks the failure memory order.