Hi all,
The builtin atomic operations (up to now) have been a little unfriendly in
simply not emitting anything if an invalid memory ordering is requested (e.g.
"__c11_atomic_load(p, memory_order_release)").
This patch adds some Sema checking (to an error) if the operand is an
integer-constant-expression and not permitted. It'll obviously still miss cases
(since we apparently support non-constant arguments there), but hopefully it's
an improvement to the current situation.
Does it look reasonable? My Clang is rather rusty.
Cheers.
Tim
http://llvm-reviews.chandlerc.com/D3027
Files:
include/clang/AST/Expr.h
include/clang/Basic/DiagnosticSemaKinds.td
lib/CodeGen/CGAtomic.cpp
lib/Sema/SemaChecking.cpp
test/CodeGen/atomic-ops.c
test/CodeGen/big-atomic-ops.c
test/Sema/atomic-ops.c
Index: include/clang/AST/Expr.h
===================================================================
--- include/clang/AST/Expr.h
+++ include/clang/AST/Expr.h
@@ -4728,6 +4728,16 @@
BI_First = 0
};
+ // The ABI values for various atomic memory orderings.
+ enum AtomicOrderingKind {
+ AO_ABI_memory_order_relaxed = 0,
+ AO_ABI_memory_order_consume = 1,
+ AO_ABI_memory_order_acquire = 2,
+ AO_ABI_memory_order_release = 3,
+ AO_ABI_memory_order_acq_rel = 4,
+ AO_ABI_memory_order_seq_cst = 5
+ };
+
private:
enum { PTR, ORDER, VAL1, ORDER_FAIL, VAL2, WEAK, END_EXPR };
Stmt* SubExprs[END_EXPR];
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -5689,7 +5689,9 @@
def err_atomic_op_bitwise_needs_atomic_int : Error<
"address argument to bitwise atomic operation must be a pointer to "
"%select{|atomic }0integer (%1 invalid)">;
-
+def err_atomic_op_has_invalid_memory_order : Error<
+ "memory order argument to atomic operation is invalid">;
+
def err_atomic_load_store_uses_lib : Error<
"atomic %select{load|store}0 requires runtime support that is not "
"available for this target">;
Index: lib/CodeGen/CGAtomic.cpp
===================================================================
--- lib/CodeGen/CGAtomic.cpp
+++ lib/CodeGen/CGAtomic.cpp
@@ -24,16 +24,6 @@
using namespace clang;
using namespace CodeGen;
-// The ABI values for various atomic memory orderings.
-enum AtomicOrderingKind {
- AO_ABI_memory_order_relaxed = 0,
- AO_ABI_memory_order_consume = 1,
- AO_ABI_memory_order_acquire = 2,
- AO_ABI_memory_order_release = 3,
- AO_ABI_memory_order_acq_rel = 4,
- AO_ABI_memory_order_seq_cst = 5
-};
-
namespace {
class AtomicInfo {
CodeGenFunction &CGF;
@@ -617,30 +607,30 @@
if (isa<llvm::ConstantInt>(Order)) {
int ord = cast<llvm::ConstantInt>(Order)->getZExtValue();
switch (ord) {
- case AO_ABI_memory_order_relaxed:
+ case AtomicExpr::AO_ABI_memory_order_relaxed:
EmitAtomicOp(*this, E, Dest, Ptr, Val1, Val2, Size, Align,
llvm::Monotonic);
break;
- case AO_ABI_memory_order_consume:
- case AO_ABI_memory_order_acquire:
+ case AtomicExpr::AO_ABI_memory_order_consume:
+ case AtomicExpr::AO_ABI_memory_order_acquire:
if (IsStore)
break; // Avoid crashing on code with undefined behavior
EmitAtomicOp(*this, E, Dest, Ptr, Val1, Val2, Size, Align,
llvm::Acquire);
break;
- case AO_ABI_memory_order_release:
+ case AtomicExpr::AO_ABI_memory_order_release:
if (IsLoad)
break; // Avoid crashing on code with undefined behavior
EmitAtomicOp(*this, E, Dest, Ptr, Val1, Val2, Size, Align,
llvm::Release);
break;
- case AO_ABI_memory_order_acq_rel:
+ case AtomicExpr::AO_ABI_memory_order_acq_rel:
if (IsLoad || IsStore)
break; // Avoid crashing on code with undefined behavior
EmitAtomicOp(*this, E, Dest, Ptr, Val1, Val2, Size, Align,
llvm::AcquireRelease);
break;
- case AO_ABI_memory_order_seq_cst:
+ case AtomicExpr::AO_ABI_memory_order_seq_cst:
EmitAtomicOp(*this, E, Dest, Ptr, Val1, Val2, Size, Align,
llvm::SequentiallyConsistent);
break;
@@ -763,8 +753,8 @@
getContext().VoidPtrTy);
args.add(RValue::get(EmitCastToVoidPtr(tempAddr)),
getContext().VoidPtrTy);
- args.add(RValue::get(llvm::ConstantInt::get(IntTy,
- AO_ABI_memory_order_seq_cst)),
+ args.add(RValue::get(llvm::ConstantInt::get(
+ IntTy, AtomicExpr::AO_ABI_memory_order_seq_cst)),
getContext().IntTy);
emitAtomicLibcall(*this, "__atomic_load", getContext().VoidTy, args);
@@ -913,8 +903,8 @@
getContext().VoidPtrTy);
args.add(RValue::get(EmitCastToVoidPtr(srcAddr)),
getContext().VoidPtrTy);
- args.add(RValue::get(llvm::ConstantInt::get(IntTy,
- AO_ABI_memory_order_seq_cst)),
+ args.add(RValue::get(llvm::ConstantInt::get(
+ IntTy, AtomicExpr::AO_ABI_memory_order_seq_cst)),
getContext().IntTy);
emitAtomicLibcall(*this, "__atomic_store", getContext().VoidTy, args);
return;
Index: lib/Sema/SemaChecking.cpp
===================================================================
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -922,6 +922,33 @@
return false;
}
+static bool isValidOrderingForOp(int64_t Ordering, AtomicExpr::AtomicOp Op) {
+ if (Ordering < AtomicExpr::AO_ABI_memory_order_relaxed ||
+ Ordering > AtomicExpr::AO_ABI_memory_order_seq_cst)
+ return false;
+
+ switch (Op) {
+ case AtomicExpr::AO__c11_atomic_init:
+ llvm_unreachable("There is no ordering argument for an init");
+
+ case AtomicExpr::AO__c11_atomic_load:
+ case AtomicExpr::AO__atomic_load_n:
+ case AtomicExpr::AO__atomic_load:
+ return Ordering != AtomicExpr::AO_ABI_memory_order_release &&
+ Ordering != AtomicExpr::AO_ABI_memory_order_acq_rel;
+
+ case AtomicExpr::AO__c11_atomic_store:
+ case AtomicExpr::AO__atomic_store:
+ case AtomicExpr::AO__atomic_store_n:
+ return Ordering != AtomicExpr::AO_ABI_memory_order_consume &&
+ Ordering != AtomicExpr::AO_ABI_memory_order_acquire &&
+ Ordering != AtomicExpr::AO_ABI_memory_order_acq_rel;
+
+ default:
+ return true;
+ }
+}
+
ExprResult Sema::SemaAtomicOpsOverloaded(ExprResult TheCallResult,
AtomicExpr::AtomicOp Op) {
CallExpr *TheCall = cast<CallExpr>(TheCallResult.get());
@@ -1210,7 +1237,16 @@
SubExprs.push_back(TheCall->getArg(3)); // Weak
break;
}
-
+
+ if (SubExprs.size() >= 2 && Form != Init) {
+ llvm::APSInt Result(32);
+ if (SubExprs[1]->isIntegerConstantExpr(Result, Context) &&
+ !isValidOrderingForOp(Result.getSExtValue(), Op))
+ return ExprError(Diag(SubExprs[1]->getLocStart(),
+ diag::err_atomic_op_has_invalid_memory_order)
+ << SubExprs[1]->getSourceRange());
+ }
+
AtomicExpr *AE = new (Context) AtomicExpr(TheCall->getCallee()->getLocStart(),
SubExprs, ResultType, Op,
TheCall->getRParenLoc());
Index: test/CodeGen/atomic-ops.c
===================================================================
--- test/CodeGen/atomic-ops.c
+++ test/CodeGen/atomic-ops.c
@@ -306,13 +306,4 @@
// CHECK: }
}
-// CHECK: @invalid_atomic
-void invalid_atomic(_Atomic(int) *i) {
- __c11_atomic_store(i, 1, memory_order_consume);
- __c11_atomic_store(i, 1, memory_order_acquire);
- __c11_atomic_store(i, 1, memory_order_acq_rel);
- __c11_atomic_load(i, memory_order_release);
- __c11_atomic_load(i, memory_order_acq_rel);
-}
-
#endif
Index: test/CodeGen/big-atomic-ops.c
===================================================================
--- test/CodeGen/big-atomic-ops.c
+++ test/CodeGen/big-atomic-ops.c
@@ -311,13 +311,4 @@
// CHECK: }
}
-// CHECK: @invalid_atomic
-void invalid_atomic(_Atomic(int) *i) {
- __c11_atomic_store(i, 1, memory_order_consume);
- __c11_atomic_store(i, 1, memory_order_acquire);
- __c11_atomic_store(i, 1, memory_order_acq_rel);
- __c11_atomic_load(i, memory_order_release);
- __c11_atomic_load(i, memory_order_acq_rel);
-}
-
#endif
Index: test/Sema/atomic-ops.c
===================================================================
--- test/Sema/atomic-ops.c
+++ test/Sema/atomic-ops.c
@@ -182,3 +182,29 @@
flag flagvar = { 0 };
PR16931(&flagvar); // expected-warning {{incompatible pointer types}}
}
+
+void memory_checks(_Atomic(int) *p, int val) {
+ (void)__c11_atomic_load(p, memory_order_relaxed);
+ (void)__c11_atomic_load(p, memory_order_acquire);
+ (void)__c11_atomic_load(p, memory_order_consume);
+ (void)__c11_atomic_load(p, memory_order_release); // expected-error {{memory order argument to atomic operation is invalid}}
+ (void)__c11_atomic_load(p, memory_order_acq_rel); // expected-error {{memory order argument to atomic operation is invalid}}
+ (void)__c11_atomic_load(p, memory_order_seq_cst);
+ (void)__c11_atomic_load(p, val);
+ (void)__c11_atomic_load(p, -1); // expected-error {{memory order argument to atomic operation is invalid}}
+ (void)__c11_atomic_load(p, 42); // expected-error {{memory order argument to atomic operation is invalid}}
+
+ (void)__c11_atomic_store(p, val, memory_order_relaxed);
+ (void)__c11_atomic_store(p, val, memory_order_acquire); // expected-error {{memory order argument to atomic operation is invalid}}
+ (void)__c11_atomic_store(p, val, memory_order_consume); // expected-error {{memory order argument to atomic operation is invalid}}
+ (void)__c11_atomic_store(p, val, memory_order_release);
+ (void)__c11_atomic_store(p, val, memory_order_acq_rel); // expected-error {{memory order argument to atomic operation is invalid}}
+ (void)__c11_atomic_store(p, val, memory_order_seq_cst);
+
+ (void)__c11_atomic_fetch_add(p, 1, memory_order_relaxed);
+ (void)__c11_atomic_fetch_add(p, 1, memory_order_acquire);
+ (void)__c11_atomic_fetch_add(p, 1, memory_order_consume);
+ (void)__c11_atomic_fetch_add(p, 1, memory_order_release);
+ (void)__c11_atomic_fetch_add(p, 1, memory_order_acq_rel);
+ (void)__c11_atomic_fetch_add(p, 1, memory_order_seq_cst);
+}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
