[Sema] Warn when returning a lambda that captures a local variable by reference

[erik.pilkington]

Summary

Previously, clang emitted no diagnostic for the following:

auto f() {
  int loc;
  return [&] { return loc; };
}

The problem being that this returns a dangling reference to the local variable 'loc'. This patch warns on this by extending -Wreturn-stack-address.

This patch also warns on the following, where the lambda is stored in a variable:

auto f() {
  int loc;
  auto lam = [&loc] {};
  return lam; // warn
}

I believe that this is OK, I can't think of any valid way of mutating lam that would make the code not return a dangling pointer to loc.

Also, this diagnoses the following, where a pointer to a local is captured via an init-capture:

auto f() {
  int local;
  return [x = &local] {}; // warn
}

But not here, because we would have to verify that the pointer in lam wasn't mutated in a previous call of the lambda:

auto f() {
  int local;
  auto lam = [x = &local] {};
  return lam; // no warn
}

Thanks for taking a look!

[rsmith]

But not here, because we would have to verify that the pointer in lam wasn't mutated in a previous call of the lambda:

Isn't that guaranteed, because the lambda is not marked mutable?

lib/Sema/SemaChecking.cpp

6594

I don't think it makes sense to use EvalVal for this. EvalVal is intended to handle the case of a glvalue that is being returned from the function, which isn't quite the same thing you want to check here -- I think you would be better off instead directly checking the captures of the lambda from here (LambdaDecl can give you a list) rather than calling this.

It also seems like the RetValExp doesn't actually matter at all here in most cases. If the closure type captures a local by reference, we should warn, regardless of the initialization expression, and likewise if it's not mutable and captures a local by address through an init-capture. For mutable lambdas with init-captures that capture by address, perhaps we could do a quick localized walk here on the RetValExp to see if it's returned directly.

6845

We prefer while (true) over for (;;) (but I agree the existing do ... while (true); is terrible).

6886–6887

This is the kind of problem I was worried about. Given:

auto &f() {
  auto lam = [] {};
  return lam;
}

... we'll currently diagnose that we're returning the address of a local, from here. But with this change applied, we lose that diagnostic because EvalVal can't tell the difference between this case (returning by reference) and returning the lambda by value.

test/SemaCXX/cxx1y-generic-lambdas-capturing.cpp

185

This diagnostic looks like it'll be confusing. Can you add a note pointing at the return statement in question (we're warning about the one on line 179, I assume)?

test/SemaCXX/cxx1y-init-captures.cpp

123–124

Do you know why this happens? It looks like EvalVal should bail out on the DeclRefExpr in the init-capture, because it refersToEnclosingVariableOrCapture.

test/SemaCXX/return-lambda-stack-addr.cpp

68–72

I'd like to see another testcase for the case when a local reference is safely captured by reference:

auto ref_ref_ok(int &r) {
  int &y = r;
  return [&] { return y; }; // no warning
}

[ABataev]

lib/Sema/SemaChecking.cpp

6588

Remove empty line

6591

Remove empty line

6592

auto *LambdaDecl

6709

const auto *V

6993–6994

auto Init, auto Cap. Local vars must start from capital letters

6996

Maybe it is better to use ranged-based loop, like for (auto &Cap : Lambda->captures())

[erik.pilkington]

This new patch rewrites the check to just use the captures in the lambda's CXXRecordDecl, instead of searching through the returned expression to find the LambdaExpr.
Thanks!

[erik.pilkington]

test/SemaCXX/cxx1y-generic-lambdas-capturing.cpp

185

OK, in the new patch we emit the warning at the 'return', and a note at the capture location.

test/SemaCXX/cxx1y-init-captures.cpp

123–124

For some reason refersToEnclosingVariableOrCapture isn't set here, which seems like a bug to me. The new patch works around this by warning only if the variable that we're capturing is declared in the same context as the return statement.

[erik.pilkington]

Ping!

[erik.pilkington]

Ping!

[erik.pilkington]

Ping!

[bgianfo]

Is there a reason why this was never merged?
Has the functionality been folded in in some other revision?

I saw a bug recently which would have been caught by exactly this, which would have been awesome.
How can we get this merged?

[erik.pilkington]

In D24639#1453795, @bgianfo wrote:

Is there a reason why this was never merged?
Has the functionality been folded in in some other revision?

I saw a bug recently which would have been caught by exactly this, which would have been awesome.
How can we get this merged?

Just never got reviewed, then I must of got distracted with other things and stopped pinging. I think it's a useful diagnostic, so would be nice to have. As far as landing this, it just needs to be rebased and reviewed. If you (or anyone else) is interested in this, feel free to commandeer this revision. Otherwise I might come back to it later.

include/clang/AST/LambdaCapture.h

public:
 
   /// \brief Determine whether this captures a variable length array bound
   /// expression.
   bool capturesVLAType() const {
     return DeclAndBits.getPointer() == nullptr &&
            !(DeclAndBits.getInt() & Capture_This);
   }
 
+  bool capturesByCopy() const { return DeclAndBits.getInt() & Capture_ByCopy; }
+
   /// \brief Retrieve the declaration of the local variable being
   /// captured.
   ///
   /// This operation is only valid if this capture is a variable capture
   /// (other than a capture of \c this).
   VarDecl *getCapturedVar() const {
     assert(capturesVariable() && "No variable available for capture");
     return static_cast<VarDecl *>(DeclAndBits.getPointer());

include/clang/Basic/DiagnosticSemaKinds.td

   InGroup<ReturnStackAddress>;
 def warn_ret_addr_label : Warning<
   "returning address of label, which is local">,
   InGroup<ReturnStackAddress>;
 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">;
+def note_ref_var_lambda_local_bind : Note<
+  "capturing local variable in lambda %0 here">;
 
 // Check for initializing a member variable with the address or a reference to
 // a constructor parameter.
 def warn_bind_ref_member_to_parameter : Warning<
   "binding reference member %0 to stack allocated parameter %1">,
   InGroup<DanglingField>;
 def warn_init_ptr_member_to_parameter_addr : Warning<
   "initializing pointer member %0 with the stack address of parameter %1">,

lib/Sema/SemaChecking.cpp

   const Expr *stackE = nullptr;
   SmallVector<const DeclRefExpr *, 8> refVars;
 
   // Perform checking for returned stack addresses, local blocks,
   // label addresses or references to temporaries.
   if (lhsType->isPointerType() ||
       (!S.getLangOpts().ObjCAutoRefCount && lhsType->isBlockPointerType())) {
     stackE = EvalAddr(RetValExp, refVars, /*ParentDecl=*/nullptr);
+
   } else if (lhsType->isReferenceType()) {
     stackE = EvalVal(RetValExp, refVars, /*ParentDecl=*/nullptr);
+
+  } else if (CXXRecordDecl *LambdaDecl = lhsType->getAsCXXRecordDecl()) {
+    if (LambdaDecl->isLambda())
+      stackE = EvalVal(RetValExp, refVars, /*ParentDecl=*/nullptr);
   }
 
   if (!stackE)
     return; // Nothing suspicious was found.
 
   // Parameters are initalized in the calling scope, so taking the address
   // of a parameter reference doesn't need a warning.
   for (auto *DRE : refVars)

   // found the problematic expression using notes.
   for (unsigned i = 0, e = refVars.size(); i != e; ++i) {
     const VarDecl *VD = cast<VarDecl>(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();
-    S.Diag(VD->getLocation(), diag::note_ref_var_local_bind)
-        << VD->getDeclName() << range;
+
+    unsigned Diag = VD->getType()->isReferenceType()
+                        ? diag::note_ref_var_local_bind
+                        : diag::note_ref_var_lambda_local_bind;
+
+    S.Diag(VD->getLocation(), Diag) << 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, 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

   switch (E->getStmtClass()) {
   case Stmt::DeclRefExprClass: {
     const DeclRefExpr *DR = cast<DeclRefExpr>(E);
 
     // If we leave the immediate function, the lifetime isn't about to end.
     if (DR->refersToEnclosingVariableOrCapture())
       return nullptr;
 
-    if (const VarDecl *V = dyn_cast<VarDecl>(DR->getDecl()))
+    if (const VarDecl *V = dyn_cast<VarDecl>(DR->getDecl())) {
       // If this is a reference variable, follow through to the expression that
       // it points to.
+      auto *RD = V->getType()->getAsCXXRecordDecl();
       if (V->hasLocalStorage() &&
-          V->getType()->isReferenceType() && V->hasInit()) {
+          (V->getType()->isReferenceType() || (RD && RD->isLambda())) &&
+          V->hasInit()) {
         // Add the reference variable to the "trail".
         refVars.push_back(DR);
         return EvalAddr(V->getInit(), refVars, ParentDecl);
       }
+    }
 
     return nullptr;
   }
 
   case Stmt::UnaryOperatorClass: {
     // The only unary operator that make sense to handle here
     // is AddrOf.  All others don't make sense as pointers.
     const UnaryOperator *U = cast<UnaryOperator>(E);

   }
 }
 
 ///  EvalVal - This function is complements EvalAddr in the mutual recursion.
 ///   See the comments for EvalAddr for more details.
 static const Expr *EvalVal(const Expr *E,
                            SmallVectorImpl<const DeclRefExpr *> &refVars,
                            const Decl *ParentDecl) {
-  do {
+  for (;;) {
     // We should only be called for evaluating non-pointer expressions, or
     // expressions with a pointer type that are not used as references but
     // instead
     // are l-values (e.g., DeclRefExpr with a pointer type).
 
     // Our "symbolic interpreter" is just a dispatch off the currently
     // viewed AST node.  We then recursively traverse the AST by calling
     // EvalAddr and EvalVal appropriately.

         return nullptr;
 
       if (const VarDecl *V = dyn_cast<VarDecl>(DR->getDecl())) {
         // Check if it refers to itself, e.g. "int& i = i;".
         if (V == ParentDecl)
           return DR;
 
         if (V->hasLocalStorage()) {
-          if (!V->getType()->isReferenceType())
+          auto *RD = V->getType()->getAsCXXRecordDecl();
+          if (!V->getType()->isReferenceType() && !(RD && RD->isLambda()))
             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, V);

 
     case Stmt::MaterializeTemporaryExprClass:
       if (const Expr *Result =
               EvalVal(cast<MaterializeTemporaryExpr>(E)->GetTemporaryExpr(),
                       refVars, ParentDecl))
         return Result;
       return E;
 
+    case Stmt::CXXConstructExprClass: {
+      auto *CE = cast<CXXConstructExpr>(E);
+
+      // We only check this node to find a construction of a C++ lambda.
+      if (!CE->getConstructor()->getParent()->isLambda())
+        return nullptr;
+
+      assert(CE->getNumArgs() == 1 && "Lambda constructor without 1 argument?");
+
+      E = CE->getArg(0)->IgnoreImplicit();
+      break;
+    }
+
+    case Stmt::LambdaExprClass: {
+      const LambdaExpr *Lambda = cast<LambdaExpr>(E);
+
+      SmallVector<const DeclRefExpr *, 8> oldRefVars(refVars.begin(),
+                                                     refVars.end());
+
+      auto init = Lambda->capture_init_begin();
+      auto cap = Lambda->capture_begin(), cap_end = Lambda->capture_end();
+
+      for (; cap != cap_end; ++cap, ++init) {
+        if (!cap->capturesVariable())
+          continue;
+
+        // There are 2 cases we want to catch here:
+        //  1. Capturing a local by reference, ie:
+        //       return [&local] {};
+        if (!cap->capturesByCopy() &&
+            cap->getCapturedVar()->hasLocalStorage()) {
+          if (auto *Res = EvalVal(*init, refVars, ParentDecl))
+            return Res;
+        }
+        //  2. Capturing a pointer to a local through an init-capture. ie:
+        //       return [ptr = &local] {};
+        else if (refVars.empty() && (*init)->getType()->isPointerType())
+          if (auto *Res = EvalAddr(*init, refVars, ParentDecl))
+            return Res;
+
+        refVars = oldRefVars;
+      }
+
+      return nullptr;
+    }
+
     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 nullptr;
     }
-  } while (true);
+  }
 }
 
 void
 Sema::CheckReturnValExpr(Expr *RetValExp, QualType lhsType,
                          SourceLocation ReturnLoc,
                          bool isObjCMethod,
                          const AttrVec *Attrs,
                          const FunctionDecl *FD) {

test/SemaCXX/cxx1y-generic-lambdas-capturing.cpp

const int x = 5;
     auto L = [=](auto a) { 
       const int z = 3;
       return [&,a](auto b) {
         const int y = 5;    
         return [=](auto c) { 
           int d[sizeof(a) == sizeof(c) || sizeof(c) == sizeof(b) ? 2 : 1];
           f(x, d);
           f(y, d);
-          f(z, d);
+          f(z, d); // expected-warning {{address of stack memory associated with local variable 'z' returned}}
           decltype(a) A = a;
           decltype(b) B = b;
           const int &i = cx.i;
         }; 
       };
     };
-    auto M = L(3)(3.5);
+    auto M = L(3)(3.5); // expected-note{{in instantiation}}
     M(3.14);
   }
 }
 namespace Test_no_capture_of_clearly_no_odr_use {
 auto foo() {
  const int x = 10; 
  auto L = [=](auto a) {
     return  [=](auto b) {

test/SemaCXX/cxx1y-init-captures.cpp

int test(T t = T{}) {
     auto L = [&z = x](T a) {
       return a;;
     };
         
   }
   { // will need to capture x in outer lambda
     const T x = 10; //expected-note {{declared}}
     auto L = [z = x](char a) { //expected-note {{begins}}
-      auto M = [&y = x](T b) { //expected-error {{cannot be implicitly captured}}
+      auto M = [&y = x](T b) { //expected-error {{cannot be implicitly captured}} expected-note{{capturing}}
         return y;
       };
-      return M;
+      // FIXME: We shouldn't emit this warning here.
+      return M; // expected-warning{{stack memory}}
     };
         
   }
   { // will need to capture x in outer lambda
     const T x = 10; 
     auto L = [=,z = x](char a) { 
       auto M = [&y = x](T b) { 
         return y;
auto L = [x, z = x](char a) {
         return y;
       };
       return M;
     };
   }
   { // will need to capture x in outer lambda
     const int x = 10; //expected-note 2{{declared}}
     auto L = [z = x](char a) { //expected-note 2{{begins}}
-      auto M = [&y = x](T b) { //expected-error 2{{cannot be implicitly captured}}
+      auto M = [&y = x](T b) { //expected-error 2{{cannot be implicitly captured}} expected-note{{capturing local}}
         return y;
       };
-      return M;
+      // FIXME: We shouldn't emit this warning here
+      return M; // expected-warning{{stack memory}}
     };     
   }
   {
     // no captures
     const T x = 10;
     auto L = [z = 
                   [z = x, &y = x](char a) { return z + y; }('a')](char a) 
       { return z; };

test/SemaCXX/return-lambda-stack-addr.cpp

+// RUN: %clang_cc1 -std=c++14 -verify -fsyntax-only %s
+
+auto basic() {
+  int local;
+
+  return [&]() {
+    (void)local; // expected-warning{{address of stack memory associated with local variable 'local' returned}}
+  };
+}
+
+auto by_copy() {
+  int local;
+
+  return [=]() {
+    (void)local; // no warning
+  };
+}
+
+auto nested() {
+  int local;
+
+  return [=] {
+    return [&]() { (void)local; }; // no warning
+  };
+}
+
+auto nested2() {
+  int local;
+
+  return [&] {
+    (void)[=] {
+      (void)local; // expected-warning{{address of stack memory associated with local variable 'local' returned}}
+    };
+  };
+}
+
+auto nested3() {
+  int local;
+  return [&] {
+    return [&] {
+      (void)local; // expected-warning{{address of stack memory associated with local variable 'local' returned}}
+    };
+  };
+}
+
+struct function {
+  template <class T>
+  function(T) {}
+};
+
+function to_function() {
+  int local;
+  return [&] { (void)local; };
+}
+
+auto lambda = [] {
+  int local;
+  return [&] { return local; }; // expected-warning{{address of stack memory associated with local variable 'local' returned}}
+};
+
+auto ref_copy() {
+  int x;
+  int &y = x;
+
+  return [=] { (void)y; };
+}
+
+auto ref_ref() {
+  int x;
+  int &y = x;              // expected-note{{binding reference variable 'y' here}}
+  return [&] { (void)y; }; // expected-warning{{address of stack memory associated with local variable 'x' returned}}
+}
+
+auto param(int p) {
+  return [&] { (void)p; }; // expected-warning{{address of stack memory associated with local variable 'p' returned}}
+}
+
+auto return_var() {
+  int local;
+  auto lam = [&local] {}; // expected-note 4 {{capturing local variable in lambda 'lam' here}}
+
+  auto &lam_ref = lam; // expected-note 2 {{binding reference variable 'lam_ref' here}}
+
+  if (0)
+    return lam; // expected-warning{{address of stack memory associated with local variable 'local' returned}}
+  else if (0)
+    return *&lam; // expected-warning{{address of stack memory associated with local variable 'local' returned}}
+  else if (0)
+    return lam_ref; // expected-warning{{address of stack memory associated with local variable 'local' returned}}
+  else
+    return *&lam_ref; // expected-warning{{address of stack memory associated with local variable 'local' returned}}
+}
+
+auto return_ptr_no_warn() {
+  int local;
+  auto lam = [ptr = &local]{};
+  return lam; // no warning
+}
+
+auto return_ptr_warn() {
+  int local;
+  return [ptr = &local]{}; // expected-warning{{address of stack memory associated with local variable 'local' returned}}
+}
+
+auto mutable_lambda_warn() {
+  int local;
+  auto mut = [&local]() mutable {}; // expected-note{{capturing local variable in lambda 'mut' here}}
+  return mut;                       // expected-warning{{address of stack memory associated with local variable 'local' returned}}
+}
+
+auto mutable_lambda_warn2() {
+  int local;
+  return [cap = &local]() mutable {}; // expected-warning{{address of stack memory associated with local variable 'local' returned}}
+}
+
+auto mutable_lambda_no_warn() {
+  int local;
+  auto mut = [cap = &local]() mutable {};
+  return mut; // no warning
+}
+
+auto ref_ptr() {
+  int local;
+  auto lam = [&local] {};      // expected-note{{capturing local variable in lambda 'lam' here}}
+  auto *const &lolwhat = &lam; // expected-note{{binding reference variable 'lolwhat' here}}
+  return *lolwhat;             // expected-warning{{address of stack memory associated with local variable 'local' returned}}
+}