[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 ↗

(On Diff #71562)

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

69–73

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 ↗

(On Diff #71562)

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 %select{address of|reference to}0 local variable %1 in lambda here">;
+def warn_ret_stack_addr_ref_lambda : Warning<
+  "%select{address of|reference to}0 stack memory associated with local "
+  "variable %1 returned through lambda capture">,
+  InGroup<ReturnStackAddress>;
 
 // 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

       dyn_cast<UnaryExprOrTypeTraitExpr>(E))
     if (SizeOf->getKind() == clang::UETT_SizeOf)
       return SizeOf->getTypeOfArgument();
 
   return QualType();
 }
 
 /// \brief Check for dangerous or invalid arguments to memset().
 ///
 /// This issues warnings on known problematic, dangerous or unspecified
 /// arguments to the standard 'memset', 'memcpy', 'memmove', and 'memcmp'
 /// function calls.
 ///
 /// \param Call The call expression to diagnose.
 void Sema::CheckMemaccessArguments(const CallExpr *Call,
                                    unsigned BId,
                                    IdentifierInfo *FnName) {

   // Although widely used, 'bzero' is not a standard function. Be more strict
   // with the argument types before allowing diagnostics and only allow the
   // form bzero(ptr, sizeof(...)).
   QualType FirstArgTy = Call->getArg(0)->IgnoreParenImpCasts()->getType();
   if (BId == Builtin::BIbzero && !FirstArgTy->getAs<PointerType>())
     return;
 
   for (unsigned ArgIdx = 0; ArgIdx != LastArg; ++ArgIdx) {
     const Expr *Dest = Call->getArg(ArgIdx)->IgnoreParenImpCasts();
     SourceRange ArgRange = Call->getArg(ArgIdx)->getSourceRange();
 
     QualType DestTy = Dest->getType();
     QualType PointeeTy;
     if (const PointerType *DestPtrTy = DestTy->getAs<PointerType>()) {
       PointeeTy = DestPtrTy->getPointeeType();
 
       // Never warn about void type pointers. This can be used to suppress
       // false positives.

       // "overwritten" if we're warning about the destination for any call
       // but memcmp; otherwise a verb appropriate to the call.
       if (ArgIdx != 0 || BId == Builtin::BImemcmp) {
         if (BId == Builtin::BImemcpy)
           OperationType = 1;
         else if(BId == Builtin::BImemmove)
           OperationType = 2;
         else if (BId == Builtin::BImemcmp)
           OperationType = 3;
       }
         
       DiagRuntimeBehavior(
         Dest->getExprLoc(), Dest,
         PDiag(diag::warn_dyn_class_memaccess)
           << (BId == Builtin::BImemcmp ? ArgIdx + 2 : ArgIdx)
           << FnName << IsContained << ContainedRD << OperationType
           << Call->getCallee()->getSourceRange());
     } else if (PointeeTy.hasNonTrivialObjCLifetime() &&
              BId != Builtin::BImemset)
       DiagRuntimeBehavior(

 
 static const Expr *EvalVal(const Expr *E,
                            SmallVectorImpl<const DeclRefExpr *> &refVars,
                            const Decl *ParentDecl);
 static const Expr *EvalAddr(const Expr *E,
                             SmallVectorImpl<const DeclRefExpr *> &refVars,
                             const Decl *ParentDecl);
 
+/// \brief Diagnose if the lambda \c RD, initialized by the returned expression
+/// \c E refers to a local variable.
+static void CheckReturnStackAddrLambda(Sema &S, const CXXRecordDecl *RD,
+                                       const Expr *E,
+                                       SourceLocation ReturnLoc) {
+  assert(RD->isLambda() && "Expected a lambda here!");
+
+  // We diagnose returning a lambda that captures a pointer to a local if it's
+  // not mutable or if it's being returned directly, because in either case we
+  // know that the pointer in the closure has not been updated.
+  bool ShouldDiagCapPtr = true;
+  if (!RD->getLambdaCallOperator()->isConst()) {
+    ShouldDiagCapPtr = false;
+    E = E->IgnoreParenImpCasts();
+    if (auto *CE = dyn_cast<CXXConstructExpr>(E))
+      if (CE->getNumArgs() && isa<LambdaExpr>(CE->getArg(0)->IgnoreImplicit()))
+        ShouldDiagCapPtr = true;
+  }
+
+  SmallVector<const DeclRefExpr *, 8> RefVars;
+  const NamedDecl *OffendingVar = nullptr;
+  const Expr *OffendingExpr = nullptr;
+  SourceLocation CaptureLoc;
+  bool IsReference;
+
+  for (const LambdaCapture &Cap : RD->captures()) {
+    if (!Cap.capturesVariable())
+      continue;
+
+    CaptureLoc = Cap.getLocation();
+    IsReference = !Cap.capturesByCopy();
+    const VarDecl *VD = Cap.getCapturedVar();
+    bool IsInitCapture = RD->getLambdaCallOperator() == VD->getDeclContext();
+    RefVars.clear();
+
+    // There are 3 cases that we want to catch here:
+    //  1. An init-capture whose initializing expression refers to a stack
+    //     address. Here, VD is the variable that stores the capture, ie 'l' in:
+    //       return [&l = local] {};
+    if (IsReference && IsInitCapture)
+      OffendingExpr = EvalVal(VD->getInit(), RefVars, nullptr);
+
+    //  2. Capturing a reference to a local. Here, VD is declaration of the
+    //     variable we're capturing.
+    else if (IsReference && !IsInitCapture && VD->isLocalVarDeclOrParm() &&
+             S.CurContext == VD->getDeclContext()) {
+      // If the captured variable is also a reference, follow through to the
+      // initializing expression.
+      if (VD->getType()->isReferenceType()) {
+        if (VD->hasInit())
+          OffendingExpr = EvalVal(VD->getInit(), RefVars, nullptr);
+      } else
+        OffendingVar = VD;
+    }
+    //  3. Explicitly capturing a pointer to a local by copy. ie:
+    //       return [ptr = &local] {};
+    else if (!IsReference && IsInitCapture && ShouldDiagCapPtr &&
+             VD->getType()->isPointerType() && VD->hasInit())
+      OffendingExpr = EvalAddr(VD->getInit(), RefVars, nullptr);
+
+    if (OffendingExpr || OffendingVar)
+      break;
+  }
+
+  if (auto *DRE = dyn_cast_or_null<DeclRefExpr>(OffendingExpr))
+    OffendingVar = DRE->getDecl();
+
+  if (!OffendingVar || OffendingVar->getDeclContext() != S.CurContext)
+    return;
+
+  S.Diag(ReturnLoc, diag::warn_ret_stack_addr_ref_lambda) << IsReference
+                                                          << OffendingVar;
+  S.Diag(CaptureLoc, diag::note_ref_var_lambda_local_bind) << IsReference
+                                                           << OffendingVar;
+}
+
 /// CheckReturnStackAddr - Check if a return statement returns the address
 ///   of a stack variable.
 static void
 CheckReturnStackAddr(Sema &S, Expr *RetValExp, QualType lhsType,
                      SourceLocation ReturnLoc) {
 
   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 (auto *LambdaDecl = lhsType->getAsCXXRecordDecl()) {
+    if (LambdaDecl->isLambda())
+      return CheckReturnStackAddrLambda(S, LambdaDecl, RetValExp, ReturnLoc);
   }
 
   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)

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

const int x = 5;
     auto M_int = L(2);
     ASSERT_NO_CAPTURES(M_int);
   }
   { // Permutations of this example must be thoroughly tested!
     const int x = 0;
     sample::X cx{5};
     auto L = [=](auto a) { 
       const int z = 3;
-      return [&,a](auto b) {
+      return [&,a](auto b) {  // expected-warning {{reference to stack memory associated with local variable 'z' returned}}
         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-note{{capturing reference to local variable 'z' in lambda here}}
           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/return-lambda-stack-addr.cpp

+// RUN: %clang_cc1 -std=c++14 -verify -fsyntax-only %s
+
+auto basic() {
+  int local;
+
+  return [&]() { // expected-warning{{reference to stack memory associated with local variable 'local' returned through lambda capture}}
+    (void)local; // expected-note{{capturing reference to local variable 'local' in lambda here}}
+  };
+}
+
+auto extern_lambda() {
+  return basic(); // no warn
+}
+
+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 [&] { // expected-warning{{reference to stack memory associated with local variable 'local' returned through lambda capture}}
+    (void)[=] {
+      (void)local; // expected-note{{capturing reference to local variable 'local' in lambda here}}
+    };
+  };
+}
+
+auto nested3() {
+  int local;
+  return [&] { // expected-warning{{reference to stack memory associated with local variable 'local' returned through lambda capture}}
+    return [&] {
+      (void)local; // expected-note{{capturing reference to local variable 'local' in lambda here}}
+    };
+  };
+}
+
+auto nested4() {
+  int local;
+  (void)[&] {
+    return [&] { (void)local; }; // no warn
+  };
+}
+
+auto lambda = [] {
+  int local;
+  return [&] { return local; }; // expected-warning{{reference to stack memory associated with local variable 'local' returned through lambda capture}} expected-note{{capturing reference to local variable 'local' in lambda here}}
+};
+
+auto ref_copy() {
+  int x;
+  int &y = x;
+
+  return [=] { (void)y; }; // no warn
+}
+
+auto ref_ref() {
+  int x;
+  int &y = x;
+  return [&] { (void)y; }; // expected-warning{{reference to stack memory associated with local variable 'x' returned through lambda capture}} expected-note{{capturing reference to local}}
+}
+
+auto ref_ref_ok(int &r) {
+  int &y = r;
+  return [&] { return y; }; // no warning
+}
+
+auto &return_ref() {
+  auto lam = [] {};
+  return lam; // expected-warning{{reference to stack memory}}
+}
+
+auto param(int p) {
+  return [&] { (void)p; }; // expected-warning{{reference to stack memory associated with local variable 'p' returned}} expected-note{{capturing reference to local}}
+}
+
+auto return_var() {
+  int local;
+  auto lam = [&local] {}; // expected-note 2 {{capturing reference to local variable 'local' in lambda here}}
+
+  auto &lam_ref = lam;
+
+  if (0)
+    return lam; // expected-warning{{reference to stack memory associated with local variable 'local' returned}}
+  else
+    return lam_ref; // expected-warning{{reference to stack memory associated with local variable 'local' returned}}
+}
+
+auto return_ptr_in_var_warn() {
+  int local;
+  auto lam = [ptr = &local]{}; // expected-note{{capturing address of local variable 'local' in lambda here}}
+  return lam; // expected-warning{{address of stack memory associated with local variable 'local' returned through lambda capture}}
+}
+
+auto return_ptr_warn() {
+  int local;
+  return [ptr = &local]{}; // expected-warning{{address of stack memory associated with local variable 'local' returned through lambda capture}} expected-note{{capturing address of local variable 'local' in lambda here}}
+}
+
+auto mutable_lambda_warn() {
+  int local;
+  auto mut = [&local]() mutable {}; // expected-note{{capturing reference to local variable 'local' in lambda here}}
+  return mut; // expected-warning{{reference to stack memory associated with local variable 'local' returned through lambda capture}}
+}
+
+auto mutable_lambda_warn2() {
+  int local;
+  return [cap = &local]() mutable {}; // expected-warning{{address of stack memory associated with local variable 'local' returned through lambda capture}} expected-note{{capturing address of local variable 'local' in lambda here}}
+}
+
+auto mutable_lambda_no_warn() {
+  int local;
+  auto mut = [cap = &local]() mutable {};
+  return mut; // no warning
+}