From: Eli Friedman Date: Thu, 24 May 2012 00:47:05 +0000 (+0000) Subject: Add a warning to diagnose statements in C++ like "*(volatile int*)x;". Conceptually... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=a6115068cde719142eb394db88612c185cabd05b;p=clang Add a warning to diagnose statements in C++ like "*(volatile int*)x;". Conceptually, this is part of -Wunused-value, but I added a separate flag -Wunused-volatile-lvalue so it doesn't get turned off by accident with -Wno-unused-value. I also made a few minor improvements to existing unused value warnings in the process. . git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@157362 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/AST/Expr.h b/include/clang/AST/Expr.h index b0b9b0fd6f..6cb34c7fb8 100644 --- a/include/clang/AST/Expr.h +++ b/include/clang/AST/Expr.h @@ -179,11 +179,12 @@ public: SourceLocation getExprLoc() const LLVM_READONLY; /// 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 - /// warning. - bool isUnusedResultAWarning(SourceLocation &Loc, SourceRange &R1, - SourceRange &R2, ASTContext &Ctx) const; + /// 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 + /// for a warning. + bool isUnusedResultAWarning(const Expr *&WarnExpr, SourceLocation &Loc, + SourceRange &R1, SourceRange &R2, + ASTContext &Ctx) const; /// isLValue - True if this expression is an "l-value" according to /// the rules of the current language. C and C++ give somewhat diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 56ecbef15f..50aa9b345f 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -4728,6 +4728,10 @@ def warn_unused_call : Warning< def warn_unused_result : Warning< "ignoring return value of function declared with warn_unused_result " "attribute">, InGroup>; +def warn_unused_volatile : Warning< + "expression result unused; assign into a variable to force a volatile load">, + InGroup>; + def warn_unused_comparison : Warning< "%select{equality|inequality}0 comparison result unused">, InGroup; diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp index ba2cc8eb34..729030b7ad 100644 --- a/lib/AST/Expr.cpp +++ b/lib/AST/Expr.cpp @@ -1654,8 +1654,9 @@ Stmt *BlockExpr::getBody() { /// 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 /// warning. -bool Expr::isUnusedResultAWarning(SourceLocation &Loc, SourceRange &R1, - SourceRange &R2, ASTContext &Ctx) const { +bool Expr::isUnusedResultAWarning(const Expr *&WarnE, SourceLocation &Loc, + SourceRange &R1, SourceRange &R2, + ASTContext &Ctx) const { // Don't warn if the expr is type dependent. The type could end up // instantiating to void. if (isTypeDependent()) @@ -1665,30 +1666,32 @@ bool Expr::isUnusedResultAWarning(SourceLocation &Loc, SourceRange &R1, default: if (getType()->isVoidType()) return false; + WarnE = this; Loc = getExprLoc(); R1 = getSourceRange(); return true; case ParenExprClass: return cast(this)->getSubExpr()-> - isUnusedResultAWarning(Loc, R1, R2, Ctx); + isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx); case GenericSelectionExprClass: return cast(this)->getResultExpr()-> - isUnusedResultAWarning(Loc, R1, R2, Ctx); + isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx); case UnaryOperatorClass: { const UnaryOperator *UO = cast(this); switch (UO->getOpcode()) { - default: break; + case UO_Plus: + case UO_Minus: + case UO_AddrOf: + case UO_Not: + case UO_LNot: + case UO_Deref: + break; case UO_PostInc: case UO_PostDec: case UO_PreInc: case UO_PreDec: // ++/-- return false; // Not a warning. - case UO_Deref: - // Dereferencing a volatile pointer is a side-effect. - if (Ctx.getCanonicalType(getType()).isVolatileQualified()) - return false; - break; case UO_Real: case UO_Imag: // accessing a piece of a volatile complex is a side-effect. @@ -1697,8 +1700,9 @@ bool Expr::isUnusedResultAWarning(SourceLocation &Loc, SourceRange &R1, return false; break; case UO_Extension: - return UO->getSubExpr()->isUnusedResultAWarning(Loc, R1, R2, Ctx); + return UO->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx); } + WarnE = this; Loc = UO->getOperatorLoc(); R1 = UO->getSubExpr()->getSourceRange(); return true; @@ -1717,17 +1721,18 @@ bool Expr::isUnusedResultAWarning(SourceLocation &Loc, SourceRange &R1, dyn_cast(BO->getRHS()->IgnoreParens())) if (IE->getValue() == 0) return false; - return BO->getRHS()->isUnusedResultAWarning(Loc, R1, R2, Ctx); + return BO->getRHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx); // Consider '||', '&&' to have side effects if the LHS or RHS does. case BO_LAnd: case BO_LOr: - if (!BO->getLHS()->isUnusedResultAWarning(Loc, R1, R2, Ctx) || - !BO->getRHS()->isUnusedResultAWarning(Loc, R1, R2, Ctx)) + if (!BO->getLHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx) || + !BO->getRHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx)) return false; break; } if (BO->isAssignmentOp()) return false; + WarnE = this; Loc = BO->getOperatorLoc(); R1 = BO->getLHS()->getSourceRange(); R2 = BO->getRHS()->getSourceRange(); @@ -1743,28 +1748,22 @@ bool Expr::isUnusedResultAWarning(SourceLocation &Loc, SourceRange &R1, // be being used for control flow. Only warn if both the LHS and // RHS are warnings. const ConditionalOperator *Exp = cast(this); - if (!Exp->getRHS()->isUnusedResultAWarning(Loc, R1, R2, Ctx)) + if (!Exp->getRHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx)) return false; if (!Exp->getLHS()) return true; - return Exp->getLHS()->isUnusedResultAWarning(Loc, R1, R2, Ctx); + return Exp->getLHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx); } case MemberExprClass: - // If the base pointer or element is to a volatile pointer/field, accessing - // it is a side effect. - if (Ctx.getCanonicalType(getType()).isVolatileQualified()) - return false; + WarnE = this; Loc = cast(this)->getMemberLoc(); R1 = SourceRange(Loc, Loc); R2 = cast(this)->getBase()->getSourceRange(); return true; case ArraySubscriptExprClass: - // If the base pointer or element is to a volatile pointer/field, accessing - // it is a side effect. - if (Ctx.getCanonicalType(getType()).isVolatileQualified()) - return false; + WarnE = this; Loc = cast(this)->getRBracketLoc(); R1 = cast(this)->getLHS()->getSourceRange(); R2 = cast(this)->getRHS()->getSourceRange(); @@ -1780,6 +1779,7 @@ bool Expr::isUnusedResultAWarning(SourceLocation &Loc, SourceRange &R1, const CXXOperatorCallExpr *Op = cast(this); if (Op->getOperator() == OO_EqualEqual || Op->getOperator() == OO_ExclaimEqual) { + WarnE = this; Loc = Op->getOperatorLoc(); R1 = Op->getSourceRange(); return true; @@ -1800,6 +1800,7 @@ bool Expr::isUnusedResultAWarning(SourceLocation &Loc, SourceRange &R1, // updated to match for QoI. if (FD->getAttr() || FD->getAttr() || FD->getAttr()) { + WarnE = this; Loc = CE->getCallee()->getLocStart(); R1 = CE->getCallee()->getSourceRange(); @@ -1824,6 +1825,7 @@ bool Expr::isUnusedResultAWarning(SourceLocation &Loc, SourceRange &R1, ME->getSelector().getIdentifierInfoForSlot(0) && ME->getSelector().getIdentifierInfoForSlot(0) ->getName().startswith("init")) { + WarnE = this; Loc = getExprLoc(); R1 = ME->getSourceRange(); return true; @@ -1831,6 +1833,7 @@ bool Expr::isUnusedResultAWarning(SourceLocation &Loc, SourceRange &R1, const ObjCMethodDecl *MD = ME->getMethodDecl(); if (MD && MD->getAttr()) { + WarnE = this; Loc = getExprLoc(); return true; } @@ -1838,6 +1841,7 @@ bool Expr::isUnusedResultAWarning(SourceLocation &Loc, SourceRange &R1, } case ObjCPropertyRefExprClass: + WarnE = this; Loc = getExprLoc(); R1 = getSourceRange(); return true; @@ -1850,6 +1854,7 @@ bool Expr::isUnusedResultAWarning(SourceLocation &Loc, SourceRange &R1, isa(PO->getSyntacticForm())) return false; + WarnE = this; Loc = getExprLoc(); R1 = getSourceRange(); return true; @@ -1864,50 +1869,70 @@ bool Expr::isUnusedResultAWarning(SourceLocation &Loc, SourceRange &R1, const CompoundStmt *CS = cast(this)->getSubStmt(); if (!CS->body_empty()) { if (const Expr *E = dyn_cast(CS->body_back())) - return E->isUnusedResultAWarning(Loc, R1, R2, Ctx); + return E->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx); if (const LabelStmt *Label = dyn_cast(CS->body_back())) if (const Expr *E = dyn_cast(Label->getSubStmt())) - return E->isUnusedResultAWarning(Loc, R1, R2, Ctx); + return E->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx); } if (getType()->isVoidType()) return false; + WarnE = this; Loc = cast(this)->getLParenLoc(); R1 = getSourceRange(); return true; } - case CStyleCastExprClass: - // If this is an explicit cast to void, allow it. People do this when they - // think they know what they're doing :). - if (getType()->isVoidType()) + case CStyleCastExprClass: { + // Ignore an explicit cast to void, as long as the operand isn't a + // volatile lvalue. + const CStyleCastExpr *CE = cast(this); + if (CE->getCastKind() == CK_ToVoid) { + if (CE->getSubExpr()->isGLValue() && + CE->getSubExpr()->getType().isVolatileQualified()) + return CE->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, + R1, R2, Ctx); return false; - Loc = cast(this)->getLParenLoc(); - R1 = cast(this)->getSubExpr()->getSourceRange(); + } + WarnE = this; + Loc = CE->getLParenLoc(); + R1 = CE->getSubExpr()->getSourceRange(); return true; + } case CXXFunctionalCastExprClass: { - if (getType()->isVoidType()) + // Ignore an explicit cast to void, as long as the operand isn't a + // volatile lvalue. + const CXXFunctionalCastExpr *CE = cast(this); + if (CE->getCastKind() == CK_ToVoid) { + if (CE->getSubExpr()->isGLValue() && + CE->getSubExpr()->getType().isVolatileQualified()) + return CE->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, + R1, R2, Ctx); return false; - const CastExpr *CE = cast(this); + } - // If this is a cast to void or a constructor conversion, check the operand. + // If this is a cast to a constructor conversion, check the operand. // Otherwise, the result of the cast is unused. - if (CE->getCastKind() == CK_ToVoid || - CE->getCastKind() == CK_ConstructorConversion) - return (cast(this)->getSubExpr() - ->isUnusedResultAWarning(Loc, R1, R2, Ctx)); - Loc = cast(this)->getTypeBeginLoc(); - R1 = cast(this)->getSubExpr()->getSourceRange(); + if (CE->getCastKind() == CK_ConstructorConversion) + return CE->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx); + WarnE = this; + Loc = CE->getTypeBeginLoc(); + R1 = CE->getSubExpr()->getSourceRange(); return true; } - case ImplicitCastExprClass: - // Check the operand, since implicit casts are inserted by Sema - return (cast(this) - ->getSubExpr()->isUnusedResultAWarning(Loc, R1, R2, Ctx)); + case ImplicitCastExprClass: { + const CastExpr *ICE = cast(this); + // lvalue-to-rvalue conversion on a volatile lvalue is a side-effect. + if (ICE->getCastKind() == CK_LValueToRValue && + ICE->getSubExpr()->getType().isVolatileQualified()) + return false; + + return ICE->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx); + } case CXXDefaultArgExprClass: return (cast(this) - ->getExpr()->isUnusedResultAWarning(Loc, R1, R2, Ctx)); + ->getExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx)); case CXXNewExprClass: // FIXME: In theory, there might be new expressions that don't have side @@ -1916,10 +1941,10 @@ bool Expr::isUnusedResultAWarning(SourceLocation &Loc, SourceRange &R1, return false; case CXXBindTemporaryExprClass: return (cast(this) - ->getSubExpr()->isUnusedResultAWarning(Loc, R1, R2, Ctx)); + ->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx)); case ExprWithCleanupsClass: return (cast(this) - ->getSubExpr()->isUnusedResultAWarning(Loc, R1, R2, Ctx)); + ->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx)); } } diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index 67b60fad51..ce4234b7b0 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -7412,8 +7412,6 @@ QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS, // C99 6.5.17 static QualType CheckCommaOperands(Sema &S, ExprResult &LHS, ExprResult &RHS, SourceLocation Loc) { - S.DiagnoseUnusedExprResult(LHS.get()); - LHS = S.CheckPlaceholderExpr(LHS.take()); RHS = S.CheckPlaceholderExpr(RHS.take()); if (LHS.isInvalid() || RHS.isInvalid()) @@ -7429,6 +7427,8 @@ static QualType CheckCommaOperands(Sema &S, ExprResult &LHS, ExprResult &RHS, if (LHS.isInvalid()) return QualType(); + S.DiagnoseUnusedExprResult(LHS.get()); + if (!S.getLangOpts().CPlusPlus) { RHS = S.DefaultFunctionArrayLvalueConversion(RHS.take()); if (RHS.isInvalid()) diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp index f64fc96c0e..a342cbd6cc 100644 --- a/lib/Sema/SemaStmt.cpp +++ b/lib/Sema/SemaStmt.cpp @@ -153,10 +153,11 @@ void Sema::DiagnoseUnusedExprResult(const Stmt *S) { if (!E) return; + const Expr *WarnExpr; SourceLocation Loc; SourceRange R1, R2; if (SourceMgr.isInSystemMacro(E->getExprLoc()) || - !E->isUnusedResultAWarning(Loc, R1, R2, Context)) + !E->isUnusedResultAWarning(WarnExpr, Loc, R1, R2, Context)) return; // Okay, we have an unused result. Depending on what the base expression is, @@ -171,7 +172,7 @@ void Sema::DiagnoseUnusedExprResult(const Stmt *S) { if (DiagnoseUnusedComparison(*this, E)) return; - E = E->IgnoreParenImpCasts(); + E = WarnExpr; if (const CallExpr *CE = dyn_cast(E)) { if (E->getType()->isVoidType()) return; @@ -229,6 +230,11 @@ void Sema::DiagnoseUnusedExprResult(const Stmt *S) { } } + if (E->isGLValue() && E->getType().isVolatileQualified()) { + Diag(Loc, diag::warn_unused_volatile) << R1 << R2; + return; + } + DiagRuntimeBehavior(Loc, 0, PDiag(DiagID) << R1 << R2); } diff --git a/test/Sema/unused-expr.c b/test/Sema/unused-expr.c index 97611168f1..d8d8795278 100644 --- a/test/Sema/unused-expr.c +++ b/test/Sema/unused-expr.c @@ -92,6 +92,7 @@ int t6() { fn2(92, 21); // expected-warning {{ignoring return value of function declared with pure attribute}} fn3(42); // expected-warning {{ignoring return value of function declared with const attribute}} __builtin_fabsf(0); // expected-warning {{ignoring return value of function declared with const attribute}} + (void)0, fn1(); // expected-warning {{ignoring return value of function declared with warn_unused_result attribute}} return 0; } diff --git a/test/SemaCXX/reinterpret-cast.cpp b/test/SemaCXX/reinterpret-cast.cpp index 7f41b93a46..a4bc4325e6 100644 --- a/test/SemaCXX/reinterpret-cast.cpp +++ b/test/SemaCXX/reinterpret-cast.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -fsyntax-only -verify -ffreestanding -Wundefined-reinterpret-cast %s +// RUN: %clang_cc1 -fsyntax-only -verify -ffreestanding -Wundefined-reinterpret-cast -Wno-unused-volatile-lvalue %s #include diff --git a/test/SemaCXX/unused.cpp b/test/SemaCXX/unused.cpp index 88783ce1a6..b330d6ecf4 100644 --- a/test/SemaCXX/unused.cpp +++ b/test/SemaCXX/unused.cpp @@ -1,24 +1,34 @@ // RUN: %clang_cc1 -fsyntax-only -verify %s + // PR4103 : Make sure we don't get a bogus unused expression warning -class APInt { - char foo; -}; -class APSInt : public APInt { - char bar; -public: - APSInt &operator=(const APSInt &RHS); -}; +namespace PR4103 { + class APInt { + char foo; + }; + class APSInt : public APInt { + char bar; + public: + APSInt &operator=(const APSInt &RHS); + }; -APSInt& APSInt::operator=(const APSInt &RHS) { - APInt::operator=(RHS); - return *this; -} + APSInt& APSInt::operator=(const APSInt &RHS) { + APInt::operator=(RHS); + return *this; + } -template -struct X { - X(); -}; + template + struct X { + X(); + }; + + void test() { + X(); + } +} -void test() { - X(); +namespace derefvolatile { + void f(volatile char* x) { + *x; // expected-warning {{expression result unused; assign into a variable to force a volatile load}} + (void)*x; // expected-warning {{expression result unused; assign into a variable to force a volatile load}} + } } diff --git a/test/SemaCXX/warn-unused-value.cpp b/test/SemaCXX/warn-unused-value.cpp index 1c0263c581..072ee60f1f 100644 --- a/test/SemaCXX/warn-unused-value.cpp +++ b/test/SemaCXX/warn-unused-value.cpp @@ -12,7 +12,7 @@ namespace test0 { // pointer to volatile has side effect (thus no warning) Box* box = new Box; box->i; // expected-warning {{expression result unused}} - box->j; + box->j; // expected-warning {{expression result unused}} } }