Page MenuHomePhabricator

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

Authored by erik.pilkington on Sep 15 2016, 3:54 PM.

Details

Reviewers
faisalv
rsmith
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!

Diff Detail

Event Timeline

erik.pilkington retitled this revision from to [Sema] Warn when returning a lambda that captures a local variable by reference.
erik.pilkington updated this object.
erik.pilkington added reviewers: faisalv, rsmith.
erik.pilkington added a subscriber: cfe-commits.
rsmith edited edge metadata.Sep 15 2016, 6:39 PM

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 added inline comments.
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 edited edge metadata.

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!

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.

bgianfo added a subscriber: bgianfo.EditedApr 3 2019, 12:10 PM

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?

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.