From a7dffe56cfe77e23fee04d55ef563ece45cc1288 Mon Sep 17 00:00:00 2001 From: Argiris Kirtzidis Date: Tue, 30 Nov 2010 22:57:32 +0000 Subject: [PATCH] Follow through references to catch returned stack addresses, local blocks, label addresses or references to temporaries, e.g: const int& g2() { int s1; int &s2 = s1; // expected-note {{binding reference variable 's2' here}} return s2; // expected-warning {{reference to stack memory associated with local variable 's1' returned}} } git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@120483 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticSemaKinds.td | 6 + lib/Sema/SemaChecking.cpp | 177 +++++++++++++++++++++-------- test/Analysis/stack-addr-ps.cpp | 86 ++++++++++++++ test/SemaCXX/address-of-temporary.cpp | 24 ++-- test/SemaCXX/rval-references.cpp | 2 +- 5 files changed, 232 insertions(+), 63 deletions(-) rewrite test/SemaCXX/address-of-temporary.cpp (74%) diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index fd049e37d..188a5b590 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -3230,10 +3230,16 @@ def warn_ret_stack_addr : Warning< "address of stack memory associated with local variable %0 returned">; def warn_ret_stack_ref : Warning< "reference to stack memory associated with local variable %0 returned">; +def warn_ret_local_temp_addr : Warning< + "returning address of local temporary object">; +def warn_ret_local_temp_ref : Warning< + "returning reference to local temporary object">; def warn_ret_addr_label : Warning< "returning address of label, which is local">; def err_ret_local_block : Error< "returning block that lives on the local stack">; +def note_ref_var_local_bind : Note< + "binding reference variable %0 here">; // For non-floating point, expressions of the form x == x or x != x diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp index ed45de53f..46c5967e5 100644 --- a/lib/Sema/SemaChecking.cpp +++ b/lib/Sema/SemaChecking.cpp @@ -1759,8 +1759,8 @@ void Sema::CheckFormatString(const StringLiteral *FExpr, //===--- CHECK: Return Address of Stack Variable --------------------------===// -static DeclRefExpr* EvalVal(Expr *E); -static DeclRefExpr* EvalAddr(Expr* E); +static Expr *EvalVal(Expr *E, llvm::SmallVectorImpl &refVars); +static Expr *EvalAddr(Expr* E, llvm::SmallVectorImpl &refVars); /// CheckReturnStackAddr - Check if a return statement returns the address /// of a stack variable. @@ -1768,45 +1768,79 @@ void Sema::CheckReturnStackAddr(Expr *RetValExp, QualType lhsType, SourceLocation ReturnLoc) { - // Perform checking for returned stack addresses. - if (lhsType->isPointerType() || lhsType->isBlockPointerType()) { - if (DeclRefExpr *DR = EvalAddr(RetValExp)) - Diag(DR->getLocStart(), diag::warn_ret_stack_addr) - << DR->getDecl()->getDeclName() << RetValExp->getSourceRange(); - - // Skip over implicit cast expressions when checking for block expressions. - RetValExp = RetValExp->IgnoreParenCasts(); + Expr *stackE = 0; + llvm::SmallVector refVars; - if (BlockExpr *C = dyn_cast(RetValExp)) - if (C->hasBlockDeclRefExprs()) - Diag(C->getLocStart(), diag::err_ret_local_block) - << C->getSourceRange(); + // Perform checking for returned stack addresses, local blocks, + // label addresses or references to temporaries. + if (lhsType->isPointerType() || lhsType->isBlockPointerType()) { + stackE = EvalAddr(RetValExp, refVars); + } else if (lhsType->isReferenceType()) { + stackE = EvalVal(RetValExp, refVars); + } - if (AddrLabelExpr *ALE = dyn_cast(RetValExp)) - Diag(ALE->getLocStart(), diag::warn_ret_addr_label) - << ALE->getSourceRange(); + if (stackE == 0) + return; // Nothing suspicious was found. - } else if (lhsType->isReferenceType()) { - // Perform checking for stack values returned by reference. - // Check for a reference to the stack - if (DeclRefExpr *DR = EvalVal(RetValExp)) - Diag(DR->getLocStart(), diag::warn_ret_stack_ref) - << DR->getDecl()->getDeclName() << RetValExp->getSourceRange(); + SourceLocation diagLoc; + SourceRange diagRange; + if (refVars.empty()) { + diagLoc = stackE->getLocStart(); + diagRange = stackE->getSourceRange(); + } else { + // We followed through a reference variable. 'stackE' contains the + // problematic expression but we will warn at the return statement pointing + // at the reference variable. We will later display the "trail" of + // reference variables using notes. + diagLoc = refVars[0]->getLocStart(); + diagRange = refVars[0]->getSourceRange(); + } + + if (DeclRefExpr *DR = dyn_cast(stackE)) { //address of local var. + Diag(diagLoc, lhsType->isReferenceType() ? diag::warn_ret_stack_ref + : diag::warn_ret_stack_addr) + << DR->getDecl()->getDeclName() << diagRange; + } else if (isa(stackE)) { // local block. + Diag(diagLoc, diag::err_ret_local_block) << diagRange; + } else if (isa(stackE)) { // address of label. + Diag(diagLoc, diag::warn_ret_addr_label) << diagRange; + } else { // local temporary. + Diag(diagLoc, lhsType->isReferenceType() ? diag::warn_ret_local_temp_ref + : diag::warn_ret_local_temp_addr) + << diagRange; + } + + // Display the "trail" of reference variables that we followed until we + // found the problematic expression using notes. + for (unsigned i = 0, e = refVars.size(); i != e; ++i) { + VarDecl *VD = cast(refVars[i]->getDecl()); + // If this var binds to another reference var, show the range of the next + // var, otherwise the var binds to the problematic expression, in which case + // show the range of the expression. + SourceRange range = (i < e-1) ? refVars[i+1]->getSourceRange() + : stackE->getSourceRange(); + Diag(VD->getLocation(), diag::note_ref_var_local_bind) + << VD->getDeclName() << range; } } /// EvalAddr - EvalAddr and EvalVal are mutually recursive functions that /// check if the expression in a return statement evaluates to an address -/// to a location on the stack. The recursion is used to traverse the +/// to a location on the stack, a local block, an address of a label, or a +/// reference to local temporary. The recursion is used to traverse the /// AST of the return expression, with recursion backtracking when we -/// encounter a subexpression that (1) clearly does not lead to the address -/// of a stack variable or (2) is something we cannot determine leads to -/// the address of a stack variable based on such local checking. +/// encounter a subexpression that (1) clearly does not lead to one of the +/// above problematic expressions (2) is something we cannot determine leads to +/// a problematic expression based on such local checking. +/// +/// Both EvalAddr and EvalVal follow through reference variables to evaluate +/// the expression that they point to. Such variables are added to the +/// 'refVars' vector so that we know what the reference variable "trail" was. /// /// EvalAddr processes expressions that are pointers that are used as /// references (and not L-values). EvalVal handles all other values. -/// At the base case of the recursion is a check for a DeclRefExpr* in -/// the refers to a stack variable. +/// At the base case of the recursion is a check for the above problematic +/// expressions. /// /// This implementation handles: /// @@ -1816,7 +1850,10 @@ Sema::CheckReturnStackAddr(Expr *RetValExp, QualType lhsType, /// * arbitrary interplay between "&" and "*" operators /// * pointer arithmetic from an address of a stack variable /// * taking the address of an array element where the array is on the stack -static DeclRefExpr* EvalAddr(Expr *E) { +static Expr *EvalAddr(Expr *E, llvm::SmallVectorImpl &refVars) { + if (E->isTypeDependent()) + return NULL; + // We should only be called for evaluating pointer expressions. assert((E->getType()->isAnyPointerType() || E->getType()->isBlockPointerType() || @@ -1829,7 +1866,23 @@ static DeclRefExpr* EvalAddr(Expr *E) { switch (E->getStmtClass()) { case Stmt::ParenExprClass: // Ignore parentheses. - return EvalAddr(cast(E)->getSubExpr()); + return EvalAddr(cast(E)->getSubExpr(), refVars); + + case Stmt::DeclRefExprClass: { + DeclRefExpr *DR = cast(E); + + if (VarDecl *V = dyn_cast(DR->getDecl())) + // If this is a reference variable, follow through to the expression that + // it points to. + if (V->hasLocalStorage() && + V->getType()->isReferenceType() && V->hasInit()) { + // Add the reference variable to the "trail". + refVars.push_back(DR); + return EvalAddr(V->getInit(), refVars); + } + + return NULL; + } case Stmt::UnaryOperatorClass: { // The only unary operator that make sense to handle here @@ -1837,7 +1890,7 @@ static DeclRefExpr* EvalAddr(Expr *E) { UnaryOperator *U = cast(E); if (U->getOpcode() == UO_AddrOf) - return EvalVal(U->getSubExpr()); + return EvalVal(U->getSubExpr(), refVars); else return NULL; } @@ -1858,7 +1911,7 @@ static DeclRefExpr* EvalAddr(Expr *E) { if (!Base->getType()->isPointerType()) Base = B->getRHS(); assert (Base->getType()->isPointerType()); - return EvalAddr(Base); + return EvalAddr(Base, refVars); } // For conditional operators we need to see if either the LHS or RHS are @@ -1870,7 +1923,7 @@ static DeclRefExpr* EvalAddr(Expr *E) { if (Expr *lhsExpr = C->getLHS()) { // In C++, we can have a throw-expression, which has 'void' type. if (!lhsExpr->getType()->isVoidType()) - if (DeclRefExpr* LHS = EvalAddr(lhsExpr)) + if (Expr* LHS = EvalAddr(lhsExpr, refVars)) return LHS; } @@ -1878,8 +1931,16 @@ static DeclRefExpr* EvalAddr(Expr *E) { if (C->getRHS()->getType()->isVoidType()) return NULL; - return EvalAddr(C->getRHS()); + return EvalAddr(C->getRHS(), refVars); } + + case Stmt::BlockExprClass: + if (cast(E)->hasBlockDeclRefExprs()) + return E; // local block. + return NULL; + + case Stmt::AddrLabelExprClass: + return E; // address of label. // For casts, we need to handle conversions from arrays to // pointer values, and pointer-to-pointer conversions. @@ -1892,9 +1953,9 @@ static DeclRefExpr* EvalAddr(Expr *E) { if (SubExpr->getType()->isPointerType() || SubExpr->getType()->isBlockPointerType() || SubExpr->getType()->isObjCQualifiedIdType()) - return EvalAddr(SubExpr); + return EvalAddr(SubExpr, refVars); else if (T->isArrayType()) - return EvalVal(SubExpr); + return EvalVal(SubExpr, refVars); else return 0; } @@ -1913,7 +1974,7 @@ static DeclRefExpr* EvalAddr(Expr *E) { case Stmt::CXXReinterpretCastExprClass: { Expr *S = cast(E)->getSubExpr(); if (S->getType()->isPointerType() || S->getType()->isBlockPointerType()) - return EvalAddr(S); + return EvalAddr(S, refVars); else return NULL; } @@ -1927,7 +1988,7 @@ static DeclRefExpr* EvalAddr(Expr *E) { /// EvalVal - This function is complements EvalAddr in the mutual recursion. /// See the comments for EvalAddr for more details. -static DeclRefExpr* EvalVal(Expr *E) { +static Expr *EvalVal(Expr *E, llvm::SmallVectorImpl &refVars) { do { // We should only be called for evaluating non-pointer expressions, or // expressions with a pointer type that are not used as references but instead @@ -1947,13 +2008,24 @@ do { } case Stmt::DeclRefExprClass: { - // DeclRefExpr: the base case. When we hit a DeclRefExpr we are looking - // at code that refers to a variable's name. We check if it has local - // storage within the function, and if so, return the expression. + // When we hit a DeclRefExpr we are looking at code that refers to a + // variable's name. If it's not a reference variable we check if it has + // local storage within the function, and if so, return the expression. DeclRefExpr *DR = cast(E); if (VarDecl *V = dyn_cast(DR->getDecl())) - if (V->hasLocalStorage() && !V->getType()->isReferenceType()) return DR; + if (V->hasLocalStorage()) { + if (!V->getType()->isReferenceType()) + return DR; + + // Reference variable, follow through to the expression that + // it points to. + if (V->hasInit()) { + // Add the reference variable to the "trail". + refVars.push_back(DR); + return EvalVal(V->getInit(), refVars); + } + } return NULL; } @@ -1971,7 +2043,7 @@ do { UnaryOperator *U = cast(E); if (U->getOpcode() == UO_Deref) - return EvalAddr(U->getSubExpr()); + return EvalAddr(U->getSubExpr(), refVars); return NULL; } @@ -1980,20 +2052,20 @@ do { // Array subscripts are potential references to data on the stack. We // retrieve the DeclRefExpr* for the array variable if it indeed // has local storage. - return EvalAddr(cast(E)->getBase()); + return EvalAddr(cast(E)->getBase(), refVars); } case Stmt::ConditionalOperatorClass: { // For conditional operators we need to see if either the LHS or RHS are - // non-NULL DeclRefExpr's. If one is non-NULL, we return it. + // non-NULL Expr's. If one is non-NULL, we return it. ConditionalOperator *C = cast(E); // Handle the GNU extension for missing LHS. if (Expr *lhsExpr = C->getLHS()) - if (DeclRefExpr *LHS = EvalVal(lhsExpr)) + if (Expr *LHS = EvalVal(lhsExpr, refVars)) return LHS; - return EvalVal(C->getRHS()); + return EvalVal(C->getRHS(), refVars); } // Accesses to members are potential references to data on the stack. @@ -2009,11 +2081,16 @@ do { if (M->getMemberDecl()->getType()->isReferenceType()) return NULL; - return EvalVal(M->getBase()); + return EvalVal(M->getBase(), refVars); } - // Everything else: we simply don't reason about them. default: + // Check that we don't return or take the address of a reference to a + // temporary. This is only useful in C++. + if (!E->isTypeDependent() && E->isRValue()) + return E; + + // Everything else: we simply don't reason about them. return NULL; } } while (true); diff --git a/test/Analysis/stack-addr-ps.cpp b/test/Analysis/stack-addr-ps.cpp index 593ba1df9..0c1ffba4f 100644 --- a/test/Analysis/stack-addr-ps.cpp +++ b/test/Analysis/stack-addr-ps.cpp @@ -6,3 +6,89 @@ const int& g() { int s; return s; // expected-warning{{reference to stack memory associated with local variable 's' returned}} } + +const int& g2() { + int s1; + int &s2 = s1; // expected-note {{binding reference variable 's2' here}} + return s2; // expected-warning {{reference to stack memory associated with local variable 's1' returned}} +} + +const int& g3() { + int s1; + int &s2 = s1; // expected-note {{binding reference variable 's2' here}} + int &s3 = s2; // expected-note {{binding reference variable 's3' here}} + return s3; // expected-warning {{reference to stack memory associated with local variable 's1' returned}} +} + +int get_value(); + +const int &get_reference1() { return get_value(); } // expected-warning {{returning reference to local temporary}} + +const int &get_reference2() { + const int &x = get_value(); // expected-note {{binding reference variable 'x' here}} + return x; // expected-warning {{returning reference to local temporary}} +} + +const int &get_reference3() { + const int &x1 = get_value(); // expected-note {{binding reference variable 'x1' here}} + const int &x2 = x1; // expected-note {{binding reference variable 'x2' here}} + return x2; // expected-warning {{returning reference to local temporary}} +} + +int global_var; +int *f1() { + int &y = global_var; + return &y; +} + +int *f2() { + int x1; + int &x2 = x1; // expected-note {{binding reference variable 'x2' here}} + return &x2; // expected-warning {{address of stack memory associated with local variable 'x1' returned}} +} + +int *f3() { + int x1; + int *const &x2 = &x1; // expected-note {{binding reference variable 'x2' here}} + return x2; // expected-warning {{address of stack memory associated with local variable 'x1' returned}} +} + +const int *f4() { + const int &x1 = get_value(); // expected-note {{binding reference variable 'x1' here}} + const int &x2 = x1; // expected-note {{binding reference variable 'x2' here}} + return &x2; // expected-warning {{returning address of local temporary}} +} + +struct S { + int x; +}; + +int *mf() { + S s1; + S &s2 = s1; // expected-note {{binding reference variable 's2' here}} + int &x = s2.x; // expected-note {{binding reference variable 'x' here}} + return &x; // expected-warning {{address of stack memory associated with local variable 's1' returned}} +} + +void *lf() { + label: + void *const &x = &&label; // expected-note {{binding reference variable 'x' here}} + return x; // expected-warning {{returning address of label, which is local}} +} + +typedef void (^bptr)(void); + +bptr bf(int j) { + __block int i; + const bptr &qq = ^{ i=0; }; // expected-note {{binding reference variable 'qq' here}} + return qq; // expected-error {{returning block that lives on the local stack}} +} + +template +struct TS { + int *get(); + int *m() { + int *&x = get(); + return x; + } +}; diff --git a/test/SemaCXX/address-of-temporary.cpp b/test/SemaCXX/address-of-temporary.cpp dissimilarity index 74% index decdc955b..eb5dee505 100644 --- a/test/SemaCXX/address-of-temporary.cpp +++ b/test/SemaCXX/address-of-temporary.cpp @@ -1,12 +1,12 @@ -// RUN: %clang_cc1 -fsyntax-only -Wno-error=address-of-temporary -verify %s -struct X { - X(); - X(int); - X(int, int); -}; - -void *f0() { return &X(); } // expected-warning{{taking the address of a temporary object}} -void *f1() { return &X(1); } // expected-warning{{taking the address of a temporary object}} -void *f2() { return &X(1, 2); } // expected-warning{{taking the address of a temporary object}} -void *f3() { return &(X)1; } // expected-warning{{taking the address of a temporary object}} - +// RUN: %clang_cc1 -fsyntax-only -Wno-error=address-of-temporary -verify %s +struct X { + X(); + X(int); + X(int, int); +}; + +void f0() { (void)&X(); } // expected-warning{{taking the address of a temporary object}} +void f1() { (void)&X(1); } // expected-warning{{taking the address of a temporary object}} +void f2() { (void)&X(1, 2); } // expected-warning{{taking the address of a temporary object}} +void f3() { (void)&(X)1; } // expected-warning{{taking the address of a temporary object}} + diff --git a/test/SemaCXX/rval-references.cpp b/test/SemaCXX/rval-references.cpp index 30622ccbf..2068642c9 100644 --- a/test/SemaCXX/rval-references.cpp +++ b/test/SemaCXX/rval-references.cpp @@ -6,7 +6,7 @@ typedef int& ilr; typedef ilr&& ilr_c2; // Collapses to int& irr ret_irr() { - return 0; + return 0; // expected-warning {{returning reference to local temporary}} } struct not_int {}; -- 2.11.4.GIT