From b02d970b4335561940dd584f6e6497ab684c788d Mon Sep 17 00:00:00 2001 From: Jan Svoboda Date: Wed, 8 Jun 2022 13:40:14 +0200 Subject: [PATCH] [clang][sema] Generate builtin operator overloads for (volatile) _Atomic types We observed a failed assert in overloaded compound-assignment operator resolution: ``` Assertion failed: (Result.isInvalid() && "C++ binary operator overloading is missing candidates!"), function CreateOverloadedBinOp, file SemaOverload.cpp, line 13944. ... frame #4: clang` clang::Sema::CreateOverloadedBinOp(..., Opc=BO_OrAssign, ..., PerformADL=true, AllowRewrittenCandidates=false, ...) at SemaOverload.cpp:13943 frame #5: clang` BuildOverloadedBinOp(..., Opc=BO_OrAssign, ...) at SemaExpr.cpp:15228 frame #6: clang` clang::Sema::BuildBinOp(..., Opc=BO_OrAssign, ...) at SemaExpr.cpp:15330 frame #7: clang` clang::Sema::ActOnBinOp(..., Kind=pipeequal, ...) at SemaExpr.cpp:15187 frame #8: clang` clang::Parser::ParseRHSOfBinaryExpression(..., MinPrec=Assignment) at ParseExpr.cpp:629 frame #9: clang` clang::Parser::ParseAssignmentExpression(..., isTypeCast=NotTypeCast) at ParseExpr.cpp:176 frame #10: clang` clang::Parser::ParseExpression(... isTypeCast=NotTypeCast) at ParseExpr.cpp:124 frame #11: clang` clang::Parser::ParseExprStatement(...) at ParseStmt.cpp:464 ``` A simple reproducer is: ``` _Atomic unsigned an_atomic_uint; enum { an_enum_value = 1 }; void enum1() { an_atomic_uint += an_enum_value; } ``` This patch fixes the issue by generating builtin operator overloads for (volatile) _Atomic types. Reviewed By: aaron.ballman Differential Revision: https://reviews.llvm.org/D125349 --- clang/include/clang/AST/Type.h | 56 ++++++++++++ clang/lib/Sema/SemaOverload.cpp | 89 +++++++++++++------ ...c-builtin-compound-assignment-overload.cpp | 55 ++++++++++++ ...c-builtin-compound-assignment-overload.cpp | 16 ++++ 4 files changed, 191 insertions(+), 25 deletions(-) create mode 100644 clang/test/CodeGenCXX/atomic-builtin-compound-assignment-overload.cpp create mode 100644 clang/test/SemaCXX/atomic-builtin-compound-assignment-overload.cpp diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h index 5784839f4f9613..1f8086301f66e5 100644 --- a/clang/include/clang/AST/Type.h +++ b/clang/include/clang/AST/Type.h @@ -265,16 +265,31 @@ class Qualifiers { bool hasOnlyConst() const { return Mask == Const; } void removeConst() { Mask &= ~Const; } void addConst() { Mask |= Const; } + Qualifiers withConst() const { + Qualifiers Qs = *this; + Qs.addConst(); + return Qs; + } bool hasVolatile() const { return Mask & Volatile; } bool hasOnlyVolatile() const { return Mask == Volatile; } void removeVolatile() { Mask &= ~Volatile; } void addVolatile() { Mask |= Volatile; } + Qualifiers withVolatile() const { + Qualifiers Qs = *this; + Qs.addVolatile(); + return Qs; + } bool hasRestrict() const { return Mask & Restrict; } bool hasOnlyRestrict() const { return Mask == Restrict; } void removeRestrict() { Mask &= ~Restrict; } void addRestrict() { Mask |= Restrict; } + Qualifiers withRestrict() const { + Qualifiers Qs = *this; + Qs.addRestrict(); + return Qs; + } bool hasCVRQualifiers() const { return getCVRQualifiers(); } unsigned getCVRQualifiers() const { return Mask & CVRMask; } @@ -609,6 +624,47 @@ class Qualifiers { static const uint32_t AddressSpaceShift = 9; }; +class QualifiersAndAtomic { + Qualifiers Quals; + bool HasAtomic; + +public: + QualifiersAndAtomic() : HasAtomic(false) {} + QualifiersAndAtomic(Qualifiers Quals, bool HasAtomic) + : Quals(Quals), HasAtomic(HasAtomic) {} + + operator Qualifiers() const { return Quals; } + + bool hasVolatile() const { return Quals.hasVolatile(); } + bool hasConst() const { return Quals.hasConst(); } + bool hasRestrict() const { return Quals.hasRestrict(); } + bool hasAtomic() const { return HasAtomic; } + + void addVolatile() { Quals.addVolatile(); } + void addConst() { Quals.addConst(); } + void addRestrict() { Quals.addRestrict(); } + void addAtomic() { HasAtomic = true; } + + void removeVolatile() { Quals.removeVolatile(); } + void removeConst() { Quals.removeConst(); } + void removeRestrict() { Quals.removeRestrict(); } + void removeAtomic() { HasAtomic = false; } + + QualifiersAndAtomic withVolatile() { + return {Quals.withVolatile(), HasAtomic}; + } + QualifiersAndAtomic withConst() { return {Quals.withConst(), HasAtomic}; } + QualifiersAndAtomic withRestrict() { + return {Quals.withRestrict(), HasAtomic}; + } + QualifiersAndAtomic withAtomic() { return {Quals, true}; } + + QualifiersAndAtomic &operator+=(Qualifiers RHS) { + Quals += RHS; + return *this; + } +}; + /// A std::pair-like structure for storing a qualified type split /// into its local qualifiers and its locally-unqualified type. struct SplitQualType { diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp index a87c31d6a213db..d1bc5373c5339b 100644 --- a/clang/lib/Sema/SemaOverload.cpp +++ b/clang/lib/Sema/SemaOverload.cpp @@ -8200,6 +8200,49 @@ static Qualifiers CollectVRQualifiers(ASTContext &Context, Expr* ArgExpr) { return VRQuals; } +// Note: We're currently only handling qualifiers that are meaningful for the +// LHS of compound assignment overloading. +static void forAllQualifierCombinationsImpl( + QualifiersAndAtomic Available, QualifiersAndAtomic Applied, + llvm::function_ref Callback) { + // _Atomic + if (Available.hasAtomic()) { + Available.removeAtomic(); + forAllQualifierCombinationsImpl(Available, Applied.withAtomic(), Callback); + forAllQualifierCombinationsImpl(Available, Applied, Callback); + return; + } + + // volatile + if (Available.hasVolatile()) { + Available.removeVolatile(); + assert(!Applied.hasVolatile()); + forAllQualifierCombinationsImpl(Available, Applied.withVolatile(), + Callback); + forAllQualifierCombinationsImpl(Available, Applied, Callback); + return; + } + + Callback(Applied); +} + +static void forAllQualifierCombinations( + QualifiersAndAtomic Quals, + llvm::function_ref Callback) { + return forAllQualifierCombinationsImpl(Quals, QualifiersAndAtomic(), + Callback); +} + +static QualType makeQualifiedLValueReferenceType(QualType Base, + QualifiersAndAtomic Quals, + Sema &S) { + if (Quals.hasAtomic()) + Base = S.Context.getAtomicType(Base); + if (Quals.hasVolatile()) + Base = S.Context.getVolatileType(Base); + return S.Context.getLValueReferenceType(Base); +} + namespace { /// Helper class to manage the addition of builtin operator overload @@ -8210,7 +8253,7 @@ class BuiltinOperatorOverloadBuilder { // Common instance state available to all overload candidate addition methods. Sema &S; ArrayRef Args; - Qualifiers VisibleTypeConversionsQuals; + QualifiersAndAtomic VisibleTypeConversionsQuals; bool HasArithmeticOrEnumeralCandidateType; SmallVectorImpl &CandidateTypes; OverloadCandidateSet &CandidateSet; @@ -8334,7 +8377,7 @@ class BuiltinOperatorOverloadBuilder { public: BuiltinOperatorOverloadBuilder( Sema &S, ArrayRef Args, - Qualifiers VisibleTypeConversionsQuals, + QualifiersAndAtomic VisibleTypeConversionsQuals, bool HasArithmeticOrEnumeralCandidateType, SmallVectorImpl &CandidateTypes, OverloadCandidateSet &CandidateSet) @@ -8955,18 +8998,14 @@ class BuiltinOperatorOverloadBuilder { ParamTypes[1] = ArithmeticTypes[Right]; auto LeftBaseTy = AdjustAddressSpaceForBuiltinOperandType( S, ArithmeticTypes[Left], Args[0]); - // Add this built-in operator as a candidate (VQ is empty). - ParamTypes[0] = S.Context.getLValueReferenceType(LeftBaseTy); - S.AddBuiltinCandidate(ParamTypes, Args, CandidateSet, - /*IsAssignmentOperator=*/isEqualOp); - // Add this built-in operator as a candidate (VQ is 'volatile'). - if (VisibleTypeConversionsQuals.hasVolatile()) { - ParamTypes[0] = S.Context.getVolatileType(LeftBaseTy); - ParamTypes[0] = S.Context.getLValueReferenceType(ParamTypes[0]); - S.AddBuiltinCandidate(ParamTypes, Args, CandidateSet, - /*IsAssignmentOperator=*/isEqualOp); - } + forAllQualifierCombinations( + VisibleTypeConversionsQuals, [&](QualifiersAndAtomic Quals) { + ParamTypes[0] = + makeQualifiedLValueReferenceType(LeftBaseTy, Quals, S); + S.AddBuiltinCandidate(ParamTypes, Args, CandidateSet, + /*IsAssignmentOperator=*/isEqualOp); + }); } } @@ -9013,16 +9052,13 @@ class BuiltinOperatorOverloadBuilder { ParamTypes[1] = ArithmeticTypes[Right]; auto LeftBaseTy = AdjustAddressSpaceForBuiltinOperandType( S, ArithmeticTypes[Left], Args[0]); - // Add this built-in operator as a candidate (VQ is empty). - ParamTypes[0] = S.Context.getLValueReferenceType(LeftBaseTy); - S.AddBuiltinCandidate(ParamTypes, Args, CandidateSet); - if (VisibleTypeConversionsQuals.hasVolatile()) { - // Add this built-in operator as a candidate (VQ is 'volatile'). - ParamTypes[0] = LeftBaseTy; - ParamTypes[0] = S.Context.getVolatileType(ParamTypes[0]); - ParamTypes[0] = S.Context.getLValueReferenceType(ParamTypes[0]); - S.AddBuiltinCandidate(ParamTypes, Args, CandidateSet); - } + + forAllQualifierCombinations( + VisibleTypeConversionsQuals, [&](QualifiersAndAtomic Quals) { + ParamTypes[0] = + makeQualifiedLValueReferenceType(LeftBaseTy, Quals, S); + S.AddBuiltinCandidate(ParamTypes, Args, CandidateSet); + }); } } } @@ -9186,10 +9222,13 @@ void Sema::AddBuiltinOperatorCandidates(OverloadedOperatorKind Op, // if the operator we're looking at has built-in operator candidates // that make use of these types. Also record whether we encounter non-record // candidate types or either arithmetic or enumeral candidate types. - Qualifiers VisibleTypeConversionsQuals; + QualifiersAndAtomic VisibleTypeConversionsQuals; VisibleTypeConversionsQuals.addConst(); - for (unsigned ArgIdx = 0, N = Args.size(); ArgIdx != N; ++ArgIdx) + for (unsigned ArgIdx = 0, N = Args.size(); ArgIdx != N; ++ArgIdx) { VisibleTypeConversionsQuals += CollectVRQualifiers(Context, Args[ArgIdx]); + if (Args[ArgIdx]->getType()->isAtomicType()) + VisibleTypeConversionsQuals.addAtomic(); + } bool HasNonRecordCandidateType = false; bool HasArithmeticOrEnumeralCandidateType = false; diff --git a/clang/test/CodeGenCXX/atomic-builtin-compound-assignment-overload.cpp b/clang/test/CodeGenCXX/atomic-builtin-compound-assignment-overload.cpp new file mode 100644 index 00000000000000..fe448383107dde --- /dev/null +++ b/clang/test/CodeGenCXX/atomic-builtin-compound-assignment-overload.cpp @@ -0,0 +1,55 @@ +// RUN: %clang_cc1 -std=gnu++11 -emit-llvm -triple=x86_64-linux-gnu -o - %s | FileCheck %s + +_Atomic unsigned an_atomic_uint; + +enum { an_enum_value = 1 }; + +// CHECK-LABEL: define {{.*}}void @_Z5enum1v() +void enum1() { + an_atomic_uint += an_enum_value; + // CHECK: atomicrmw add ptr +} + +// CHECK-LABEL: define {{.*}}void @_Z5enum2v() +void enum2() { + an_atomic_uint |= an_enum_value; + // CHECK: atomicrmw or ptr +} + +// CHECK-LABEL: define {{.*}}void @_Z5enum3RU7_Atomicj({{.*}}) +void enum3(_Atomic unsigned &an_atomic_uint_param) { + an_atomic_uint_param += an_enum_value; + // CHECK: atomicrmw add ptr +} + +// CHECK-LABEL: define {{.*}}void @_Z5enum4RU7_Atomicj({{.*}}) +void enum4(_Atomic unsigned &an_atomic_uint_param) { + an_atomic_uint_param |= an_enum_value; + // CHECK: atomicrmw or ptr +} + +volatile _Atomic unsigned an_volatile_atomic_uint; + +// CHECK-LABEL: define {{.*}}void @_Z5enum5v() +void enum5() { + an_volatile_atomic_uint += an_enum_value; + // CHECK: atomicrmw add ptr +} + +// CHECK-LABEL: define {{.*}}void @_Z5enum6v() +void enum6() { + an_volatile_atomic_uint |= an_enum_value; + // CHECK: atomicrmw or ptr +} + +// CHECK-LABEL: define {{.*}}void @_Z5enum7RVU7_Atomicj({{.*}}) +void enum7(volatile _Atomic unsigned &an_volatile_atomic_uint_param) { + an_volatile_atomic_uint_param += an_enum_value; + // CHECK: atomicrmw add ptr +} + +// CHECK-LABEL: define {{.*}}void @_Z5enum8RVU7_Atomicj({{.*}}) +void enum8(volatile _Atomic unsigned &an_volatile_atomic_uint_param) { + an_volatile_atomic_uint_param |= an_enum_value; + // CHECK: atomicrmw or ptr +} diff --git a/clang/test/SemaCXX/atomic-builtin-compound-assignment-overload.cpp b/clang/test/SemaCXX/atomic-builtin-compound-assignment-overload.cpp new file mode 100644 index 00000000000000..b2bc5f84b2779a --- /dev/null +++ b/clang/test/SemaCXX/atomic-builtin-compound-assignment-overload.cpp @@ -0,0 +1,16 @@ +// RUN: %clang_cc1 -std=gnu++98 -fsyntax-only -verify %s +// expected-no-diagnostics + +_Atomic unsigned an_atomic_uint; + +enum { an_enum_value = 1 }; + +void enum1() { an_atomic_uint += an_enum_value; } + +void enum2() { an_atomic_uint |= an_enum_value; } + +volatile _Atomic unsigned an_volatile_atomic_uint; + +void enum3() { an_volatile_atomic_uint += an_enum_value; } + +void enum4() { an_volatile_atomic_uint |= an_enum_value; }