From 00e5d38d40162d049f67b436ad42c9d05092e65c Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Wed, 27 May 2020 17:20:15 -0700 Subject: [PATCH] Do not warn that an expression of the form (void)arr; is unused when arr is a volatile non-local array. This fixes a recent regression exposed by removing lvalue-to-rvalue conversion of discarded volatile arrays. In passing, regularize the rules we use to determine whether '(void)expr;' warns when expr is a volatile glvalue. --- clang/include/clang/AST/Expr.h | 5 ++ clang/lib/AST/Expr.cpp | 93 +++++++++++++++++++++++++++----- clang/lib/Sema/SemaExprCXX.cpp | 78 ++++----------------------- clang/test/SemaCXX/warn-unused-value.cpp | 30 +++++++++++ 4 files changed, 126 insertions(+), 80 deletions(-) diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h index 0ca4941789e..deca0b82c4e 100644 --- a/clang/include/clang/AST/Expr.h +++ b/clang/include/clang/AST/Expr.h @@ -232,6 +232,11 @@ public: /// a problem with a generic expression. SourceLocation getExprLoc() const LLVM_READONLY; + /// Determine whether an lvalue-to-rvalue conversion should implicitly be + /// applied to this expression if it appears as a discarded-value expression + /// in C++11 onwards. This applies to certain forms of volatile glvalues. + bool isReadIfDiscardedInCPlusPlus11() const; + /// isUnusedResultAWarning - Return true if this immediate expression should /// be warned about if the result is unused. If so, fill in expr, location, /// and ranges with expr to warn on and source locations/ranges appropriate diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp index 4c175fff642..feb0517204c 100644 --- a/clang/lib/AST/Expr.cpp +++ b/clang/lib/AST/Expr.cpp @@ -2267,6 +2267,64 @@ Stmt *BlockExpr::getBody() { // Generic Expression Routines //===----------------------------------------------------------------------===// +bool Expr::isReadIfDiscardedInCPlusPlus11() const { + // In C++11, discarded-value expressions of a certain form are special, + // according to [expr]p10: + // The lvalue-to-rvalue conversion (4.1) is applied only if the + // expression is an lvalue of volatile-qualified type and it has + // one of the following forms: + if (!isGLValue() || !getType().isVolatileQualified()) + return false; + + const Expr *E = IgnoreParens(); + + // - id-expression (5.1.1), + if (isa(E)) + return true; + + // - subscripting (5.2.1), + if (isa(E)) + return true; + + // - class member access (5.2.5), + if (isa(E)) + return true; + + // - indirection (5.3.1), + if (auto *UO = dyn_cast(E)) + if (UO->getOpcode() == UO_Deref) + return true; + + if (auto *BO = dyn_cast(E)) { + // - pointer-to-member operation (5.5), + if (BO->isPtrMemOp()) + return true; + + // - comma expression (5.18) where the right operand is one of the above. + if (BO->getOpcode() == BO_Comma) + return BO->getRHS()->isReadIfDiscardedInCPlusPlus11(); + } + + // - conditional expression (5.16) where both the second and the third + // operands are one of the above, or + if (auto *CO = dyn_cast(E)) + return CO->getTrueExpr()->isReadIfDiscardedInCPlusPlus11() && + CO->getFalseExpr()->isReadIfDiscardedInCPlusPlus11(); + // The related edge case of "*x ?: *x". + if (auto *BCO = + dyn_cast(E)) { + if (auto *OVE = dyn_cast(BCO->getTrueExpr())) + return OVE->getSourceExpr()->isReadIfDiscardedInCPlusPlus11() && + BCO->getFalseExpr()->isReadIfDiscardedInCPlusPlus11(); + } + + // Objective-C++ extensions to the rule. + if (isa(E) || isa(E)) + return true; + + return false; +} + /// isUnusedResultAWarning - Return true if this immediate expression should /// be warned about if the result is unused. If so, fill in Loc and Ranges /// with location to warn on and the source range[s] to report with the @@ -2555,20 +2613,31 @@ bool Expr::isUnusedResultAWarning(const Expr *&WarnE, SourceLocation &Loc, } case CXXFunctionalCastExprClass: case CStyleCastExprClass: { - // Ignore an explicit cast to void unless the operand is a non-trivial - // volatile lvalue. + // Ignore an explicit cast to void, except in C++98 if the operand is a + // volatile glvalue for which we would trigger an implicit read in any + // other language mode. (Such an implicit read always happens as part of + // the lvalue conversion in C, and happens in C++ for expressions of all + // forms where it seems likely the user intended to trigger a volatile + // load.) const CastExpr *CE = cast(this); + const Expr *SubE = CE->getSubExpr()->IgnoreParens(); if (CE->getCastKind() == CK_ToVoid) { - if (CE->getSubExpr()->isGLValue() && - CE->getSubExpr()->getType().isVolatileQualified()) { - const DeclRefExpr *DRE = - dyn_cast(CE->getSubExpr()->IgnoreParens()); - if (!(DRE && isa(DRE->getDecl()) && - cast(DRE->getDecl())->hasLocalStorage()) && - !isa(CE->getSubExpr()->IgnoreParens())) { - return CE->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, - R1, R2, Ctx); - } + if (Ctx.getLangOpts().CPlusPlus && !Ctx.getLangOpts().CPlusPlus11 && + SubE->isReadIfDiscardedInCPlusPlus11()) { + // Suppress the "unused value" warning for idiomatic usage of + // '(void)var;' used to suppress "unused variable" warnings. + if (auto *DRE = dyn_cast(SubE)) + if (auto *VD = dyn_cast(DRE->getDecl())) + if (!VD->isExternallyVisible()) + return false; + + // The lvalue-to-rvalue conversion would have no effect for an array. + // It's implausible that the programmer expected this to result in a + // volatile array load, so don't warn. + if (SubE->getType()->isArrayType()) + return false; + + return SubE->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx); } return false; } diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp index 7cda60ba759..b655c828168 100644 --- a/clang/lib/Sema/SemaExprCXX.cpp +++ b/clang/lib/Sema/SemaExprCXX.cpp @@ -7660,61 +7660,6 @@ ExprResult Sema::ActOnNoexceptExpr(SourceLocation KeyLoc, SourceLocation, return BuildCXXNoexceptExpr(KeyLoc, Operand, RParen); } -static bool IsSpecialDiscardedValue(Expr *E) { - // In C++11, discarded-value expressions of a certain form are special, - // according to [expr]p10: - // The lvalue-to-rvalue conversion (4.1) is applied only if the - // expression is an lvalue of volatile-qualified type and it has - // one of the following forms: - E = E->IgnoreParens(); - - // - id-expression (5.1.1), - if (isa(E)) - return true; - - // - subscripting (5.2.1), - if (isa(E)) - return true; - - // - class member access (5.2.5), - if (isa(E)) - return true; - - // - indirection (5.3.1), - if (UnaryOperator *UO = dyn_cast(E)) - if (UO->getOpcode() == UO_Deref) - return true; - - if (BinaryOperator *BO = dyn_cast(E)) { - // - pointer-to-member operation (5.5), - if (BO->isPtrMemOp()) - return true; - - // - comma expression (5.18) where the right operand is one of the above. - if (BO->getOpcode() == BO_Comma) - return IsSpecialDiscardedValue(BO->getRHS()); - } - - // - conditional expression (5.16) where both the second and the third - // operands are one of the above, or - if (ConditionalOperator *CO = dyn_cast(E)) - return IsSpecialDiscardedValue(CO->getTrueExpr()) && - IsSpecialDiscardedValue(CO->getFalseExpr()); - // The related edge case of "*x ?: *x". - if (BinaryConditionalOperator *BCO = - dyn_cast(E)) { - if (OpaqueValueExpr *OVE = dyn_cast(BCO->getTrueExpr())) - return IsSpecialDiscardedValue(OVE->getSourceExpr()) && - IsSpecialDiscardedValue(BCO->getFalseExpr()); - } - - // Objective-C++ extensions to the rule. - if (isa(E) || isa(E)) - return true; - - return false; -} - /// Perform the conversions required for an expression used in a /// context that ignores the result. ExprResult Sema::IgnoredValueConversions(Expr *E) { @@ -7739,23 +7684,20 @@ ExprResult Sema::IgnoredValueConversions(Expr *E) { return E; } - if (getLangOpts().CPlusPlus) { + if (getLangOpts().CPlusPlus) { // The C++11 standard defines the notion of a discarded-value expression; // normally, we don't need to do anything to handle it, but if it is a // volatile lvalue with a special form, we perform an lvalue-to-rvalue // conversion. - if (getLangOpts().CPlusPlus11 && E->isGLValue() && - 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); - } + if (getLangOpts().CPlusPlus11 && E->isReadIfDiscardedInCPlusPlus11()) { + 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: diff --git a/clang/test/SemaCXX/warn-unused-value.cpp b/clang/test/SemaCXX/warn-unused-value.cpp index 98e2a4e8630..02bceeca133 100644 --- a/clang/test/SemaCXX/warn-unused-value.cpp +++ b/clang/test/SemaCXX/warn-unused-value.cpp @@ -108,3 +108,33 @@ void f() { (void)sizeof(*x); // Ok } } + +static volatile char var1 = 'a'; +volatile char var2 = 'a'; +static volatile char arr1[] = "hello"; +volatile char arr2[] = "hello"; +void volatile_array() { + static volatile char var3 = 'a'; + volatile char var4 = 'a'; + static volatile char arr3[] = "hello"; + volatile char arr4[] = "hello"; + + // These all result in volatile loads in C and C++11. In C++98, they don't, + // but we suppress the warning in the case where '(void)var;' might be + // idiomatically suppressing an 'unused variable' warning. + (void)var1; + (void)var2; +#if __cplusplus < 201103L + // expected-warning@-2 {{expression result unused; assign into a variable to force a volatile load}} +#endif + (void)var3; + (void)var4; + + // None of these result in volatile loads in any language mode, and it's not + // really reasonable to assume that they would, since volatile array loads + // don't really exist anywhere. + (void)arr1; + (void)arr2; + (void)arr3; + (void)arr4; +} -- 2.11.0