From 4a6861a7e5b59be24a09b8b9782255d028e7aade Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Wed, 9 Oct 2019 02:04:54 +0000 Subject: [PATCH] [c++20] P1152R4: warn on any simple-assignment to a volatile lvalue whose value is not ignored. We don't warn on all the cases that are deprecated: specifically, we choose to not warn for now if there are parentheses around the assignment but its value is not actually used. This seems like a more defensible rule, particularly for cases like sizeof(v = a), where the parens are part of the operand rather than the sizeof syntax. llvm-svn: 374135 --- clang/include/clang/Basic/DiagnosticSemaKinds.td | 3 ++ clang/include/clang/Sema/Sema.h | 8 ++++ clang/lib/Sema/SemaExpr.cpp | 51 ++++++++++++++++++++---- clang/lib/Sema/SemaExprCXX.cpp | 40 +++++++++++++++---- clang/test/SemaCXX/deprecated.cpp | 49 +++++++++++++++++------ clang/www/cxx_status.html | 2 +- 6 files changed, 124 insertions(+), 29 deletions(-) diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 9d1475d2..bcd059b 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -6654,6 +6654,9 @@ def err_increment_decrement_enum : Error< def warn_deprecated_increment_decrement_volatile : Warning< "%select{decrement|increment}0 of object of volatile-qualified type %1 " "is deprecated">, InGroup; +def warn_deprecated_simple_assign_volatile : Warning< + "use of result of assignment to object of volatile-qualified type %0 " + "is deprecated">, InGroup; def warn_deprecated_compound_assign_volatile : Warning< "compound assignment to object of volatile-qualified type %0 is deprecated">, InGroup; diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index d5d91e3..c9211eb 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -1062,6 +1062,11 @@ public: llvm::SmallPtrSet PossibleDerefs; + /// Expressions appearing as the LHS of a volatile assignment in this + /// context. We produce a warning for these when popping the context if + /// they are not discarded-value expressions nor unevaluated operands. + SmallVector VolatileAssignmentLHSs; + /// \brief Describes whether we are in an expression constext which we have /// to handle differently. enum ExpressionKind { @@ -4248,6 +4253,9 @@ public: ExprResult TransformToPotentiallyEvaluated(Expr *E); ExprResult HandleExprEvaluationContextForTypeof(Expr *E); + ExprResult CheckUnevaluatedOperand(Expr *E); + void CheckUnusedVolatileAssignment(Expr *E); + ExprResult ActOnConstantExpression(ExprResult Res); // Functions for marking a declaration referenced. These functions also diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index b691668..23d3171 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -3801,6 +3801,16 @@ bool Sema::CheckUnaryExprOrTypeTraitOperand(Expr *E, QualType ExprTy = E->getType(); assert(!ExprTy->isReferenceType()); + bool IsUnevaluatedOperand = + (ExprKind == UETT_SizeOf || ExprKind == UETT_AlignOf || + ExprKind == UETT_PreferredAlignOf); + if (IsUnevaluatedOperand) { + ExprResult Result = CheckUnevaluatedOperand(E); + if (Result.isInvalid()) + return true; + E = Result.get(); + } + if (ExprKind == UETT_VecStep) return CheckVecStepTraitOperandType(*this, ExprTy, E->getExprLoc(), E->getSourceRange()); @@ -3838,9 +3848,8 @@ bool Sema::CheckUnaryExprOrTypeTraitOperand(Expr *E, // The operand for sizeof and alignof is in an unevaluated expression context, // so side effects could result in unintended consequences. - if ((ExprKind == UETT_SizeOf || ExprKind == UETT_AlignOf || - ExprKind == UETT_PreferredAlignOf) && - !inTemplateInstantiation() && E->HasSideEffects(Context, false)) + if (IsUnevaluatedOperand && !inTemplateInstantiation() && + E->HasSideEffects(Context, false)) Diag(E->getExprLoc(), diag::warn_side_effects_unevaluated_context); if (CheckObjCTraitOperandConstraints(*this, ExprTy, E->getExprLoc(), @@ -3939,8 +3948,6 @@ bool Sema::CheckUnaryExprOrTypeTraitOperand(QualType ExprType, } static bool CheckAlignOfExpr(Sema &S, Expr *E, UnaryExprOrTypeTrait ExprKind) { - E = E->IgnoreParens(); - // Cannot know anything else if the expression is dependent. if (E->isTypeDependent()) return false; @@ -3952,9 +3959,10 @@ static bool CheckAlignOfExpr(Sema &S, Expr *E, UnaryExprOrTypeTrait ExprKind) { } ValueDecl *D = nullptr; - if (DeclRefExpr *DRE = dyn_cast(E)) { + Expr *Inner = E->IgnoreParens(); + if (DeclRefExpr *DRE = dyn_cast(Inner)) { D = DRE->getDecl(); - } else if (MemberExpr *ME = dyn_cast(E)) { + } else if (MemberExpr *ME = dyn_cast(Inner)) { D = ME->getMemberDecl(); } @@ -11944,7 +11952,7 @@ QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS, // A simple-assignment whose left operand is of a volatile-qualified // type is deprecated unless the assignment is either a discarded-value // expression or an unevaluated operand - // FIXME: Implement checks for this. + ExprEvalContexts.back().VolatileAssignmentLHSs.push_back(LHSExpr); } else { // C++2a [expr.ass]p6: // [Compound-assignment] expressions are deprecated if E1 has @@ -15082,6 +15090,26 @@ void Sema::WarnOnPendingNoDerefs(ExpressionEvaluationContextRecord &Rec) { Rec.PossibleDerefs.clear(); } +/// Check whether E, which is either a discarded-value expression or an +/// unevaluated operand, is a simple-assignment to a volatlie-qualified lvalue, +/// and if so, remove it from the list of volatile-qualified assignments that +/// we are going to warn are deprecated. +void Sema::CheckUnusedVolatileAssignment(Expr *E) { + if (!E->getType().isVolatileQualified() || !getLangOpts().CPlusPlus2a) + return; + + // Note: ignoring parens here is not justified by the standard rules, but + // ignoring parentheses seems like a more reasonable approach, and this only + // drives a deprecation warning so doesn't affect conformance. + if (auto *BO = dyn_cast(E->IgnoreParenImpCasts())) { + if (BO->getOpcode() == BO_Assign) { + auto &LHSs = ExprEvalContexts.back().VolatileAssignmentLHSs; + LHSs.erase(std::remove(LHSs.begin(), LHSs.end(), BO->getLHS()), + LHSs.end()); + } + } +} + void Sema::PopExpressionEvaluationContext() { ExpressionEvaluationContextRecord& Rec = ExprEvalContexts.back(); unsigned NumTypos = Rec.NumTypos; @@ -15116,6 +15144,13 @@ void Sema::PopExpressionEvaluationContext() { WarnOnPendingNoDerefs(Rec); + // Warn on any volatile-qualified simple-assignments that are not discarded- + // value expressions nor unevaluated operands (those cases get removed from + // this list by CheckUnusedVolatileAssignment). + for (auto *BO : Rec.VolatileAssignmentLHSs) + Diag(BO->getBeginLoc(), diag::warn_deprecated_simple_assign_volatile) + << BO->getType(); + // When are coming out of an unevaluated context, clear out any // temporaries that we may have created as part of the evaluation of // the expression in that context: they aren't relevant because they diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp index 476afce..68fe529 100644 --- a/clang/lib/Sema/SemaExprCXX.cpp +++ b/clang/lib/Sema/SemaExprCXX.cpp @@ -499,6 +499,11 @@ ExprResult Sema::BuildCXXTypeId(QualType TypeInfoType, } } + ExprResult Result = CheckUnevaluatedOperand(E); + if (Result.isInvalid()) + return ExprError(); + E = Result.get(); + // C++ [expr.typeid]p4: // [...] If the type of the type-id is a reference to a possibly // cv-qualified type, the result of the typeid expression refers to a @@ -6609,6 +6614,11 @@ ExprResult Sema::ActOnDecltypeExpression(Expr *E) { ExprEvalContexts.back().ExprContext = ExpressionEvaluationContextRecord::EK_Other; + Result = CheckUnevaluatedOperand(E); + if (Result.isInvalid()) + return ExprError(); + E = Result.get(); + // In MS mode, don't perform any extra checking of call return types within a // decltype expression. if (getLangOpts().MSVCCompat) @@ -7233,7 +7243,10 @@ ExprResult Sema::BuildCXXNoexceptExpr(SourceLocation KeyLoc, Expr *Operand, if (R.isInvalid()) return R; - // The operand may have been modified when checking the placeholder type. + R = CheckUnevaluatedOperand(R.get()); + if (R.isInvalid()) + return ExprError(); + Operand = R.get(); if (!inTemplateInstantiation() && Operand->HasSideEffects(Context, false)) { @@ -7337,12 +7350,17 @@ ExprResult Sema::IgnoredValueConversions(Expr *E) { // volatile lvalue with a special form, we perform an lvalue-to-rvalue // conversion. if (getLangOpts().CPlusPlus11 && E->isGLValue() && - E->getType().isVolatileQualified() && - IsSpecialDiscardedValue(E)) { - ExprResult Res = DefaultLvalueConversion(E); - if (Res.isInvalid()) - return E; - E = Res.get(); + E->getType().isVolatileQualified()) { + if (IsSpecialDiscardedValue(E)) { + ExprResult Res = DefaultLvalueConversion(E); + if (Res.isInvalid()) + return E; + E = Res.get(); + } else { + // Per C++2a [expr.ass]p5, a volatile assignment is not deprecated if + // it occurs as a discarded-value expression. + CheckUnusedVolatileAssignment(E); + } } // C++1z: @@ -7377,6 +7395,14 @@ ExprResult Sema::IgnoredValueConversions(Expr *E) { return E; } +ExprResult Sema::CheckUnevaluatedOperand(Expr *E) { + // Per C++2a [expr.ass]p5, a volatile assignment is not deprecated if + // it occurs as an unevaluated operand. + CheckUnusedVolatileAssignment(E); + + return E; +} + // If we can unambiguously determine whether Var can never be used // in a constant expression, return true. // - if the variable and its initializer are non-dependent, then diff --git a/clang/test/SemaCXX/deprecated.cpp b/clang/test/SemaCXX/deprecated.cpp index 36c73b0..de9e424 100644 --- a/clang/test/SemaCXX/deprecated.cpp +++ b/clang/test/SemaCXX/deprecated.cpp @@ -1,13 +1,17 @@ -// RUN: %clang_cc1 -std=c++98 %s -Wdeprecated -verify -triple x86_64-linux-gnu -// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated -verify -triple x86_64-linux-gnu -// RUN: %clang_cc1 -std=c++14 %s -Wdeprecated -verify -triple x86_64-linux-gnu -// RUN: %clang_cc1 -std=c++17 %s -Wdeprecated -verify -triple x86_64-linux-gnu -// RUN: %clang_cc1 -std=c++2a %s -Wdeprecated -verify=expected,cxx20 -triple x86_64-linux-gnu +// RUN: %clang_cc1 -std=c++98 %s -Wno-parentheses -Wdeprecated -verify -triple x86_64-linux-gnu +// RUN: %clang_cc1 -std=c++11 %s -Wno-parentheses -Wdeprecated -verify -triple x86_64-linux-gnu +// RUN: %clang_cc1 -std=c++14 %s -Wno-parentheses -Wdeprecated -verify -triple x86_64-linux-gnu +// RUN: %clang_cc1 -std=c++17 %s -Wno-parentheses -Wdeprecated -verify -triple x86_64-linux-gnu +// RUN: %clang_cc1 -std=c++2a %s -Wno-parentheses -Wdeprecated -verify=expected,cxx20 -triple x86_64-linux-gnu -// RUN: %clang_cc1 -std=c++14 %s -Wdeprecated -verify -triple x86_64-linux-gnu -Wno-deprecated-register -DNO_DEPRECATED_FLAGS +// RUN: %clang_cc1 -std=c++14 %s -Wno-parentheses -Wdeprecated -verify -triple x86_64-linux-gnu -Wno-deprecated-register -DNO_DEPRECATED_FLAGS #include "Inputs/register.h" +namespace std { + struct type_info {}; +} + void g() throw(); void h() throw(int); void i() throw(...); @@ -133,17 +137,36 @@ namespace DeprecatedVolatile { n = 5; // ok #if __cplusplus >= 201103L decltype(n = 5) m = n; // ok expected-warning {{side effects}} - m = sizeof(n = 5); // ok expected-warning {{side effects}} + (void)noexcept(n = 5); // ok expected-warning {{side effects}} #endif + (void)typeid(n = 5); // ok expected-warning {{side effects}} (n = 5, 0); // ok - use(n = 5); // FIXME: deprecated - (n = 5); // FIXME: deprecated - int q = n = 5; // FIXME: deprecated - q = n = 5; // FIXME: deprecated + use(n = 5); // cxx20-warning {{use of result of assignment to object of volatile-qualified type 'volatile int' is deprecated}} + int q = n = 5; // cxx20-warning {{deprecated}} + q = n = 5; // cxx20-warning {{deprecated}} #if __cplusplus >= 201103L - decltype(q = n = 5) m2 = q; // FIXME: deprecated expected-warning {{side effects}} + decltype(q = n = 5) m2 = q; // cxx20-warning {{deprecated}} expected-warning {{side effects}} + (void)noexcept(q = n = 5); // cxx20-warning {{deprecated}} expected-warning {{side effects}} #endif - q = sizeof(q = n = 5); // FIXME: deprecated expected-warning {{side effects}} + (void)sizeof(q = n = 5); // cxx20-warning {{deprecated}} expected-warning {{side effects}} + (void)typeid(use(n = 5)); // cxx20-warning {{deprecated}} expected-warning {{side effects}} + (void)__alignof(+(n = 5)); // cxx20-warning {{deprecated}} expected-warning {{side effects}} + + // FIXME: These cases are technically deprecated because the parens are + // part of the operand, but we choose to not diagnose for now. + (void)sizeof(n = 5); // expected-warning {{side effects}} + (void)__alignof(n = 5); // expected-warning {{side effects}} + // Similarly here. + (n = 5); + + volatile bool b = true; + if (b = true) {} // cxx20-warning {{deprecated}} + for (b = true; + b = true; // cxx20-warning {{deprecated}} + b = true) {} + for (volatile bool x = true; + volatile bool y = true; // ok despite volatile load from volatile initialization + ) {} // inc / dec / compound assignments are always deprecated ++n; // cxx20-warning {{increment of object of volatile-qualified type 'volatile int' is deprecated}} diff --git a/clang/www/cxx_status.html b/clang/www/cxx_status.html index 0c6f3de..322c49a 100755 --- a/clang/www/cxx_status.html +++ b/clang/www/cxx_status.html @@ -1103,7 +1103,7 @@ as the draft C++2a standard evolves. Deprecate some problematic uses of volatile P1152R4 - Partial + SVN [[nodiscard("with reason")]] -- 2.7.4