Page MenuHomePhabricator

[Sema] Diagnose local variables and parameters captured by lambda and block expressions in a default argument
Needs ReviewPublic

Authored by ahatanak on Aug 18 2017, 7:23 PM.

Details

Summary

This patch fixes an assertion failure that occurs when compiling the following invalid code:

struct S {
  template<class T>
  S(T &&) {}
};

template<class T>
void foo1(int a0, S a1 = [](){ (void)&a0; } ) { // a0 cannot be used in the default argument for a1
}

void foo2() {
  foo1<int>(1);
}

$ clang++ -std=c++14 -c -o /dev/null test.cpp

Assertion failed: (isa<LabelDecl>(D) && "declaration not instantiated in this scope"), function findInstantiationOf, file llvm/tools/clang/lib/Sema/SemaTemplateInstantiate.cpp, line 2911.

The assertion fails when findInstantiationOf is called to find the instantiated decl of a0 when instantiating the lambda expression that is the default argument for a1.

To prevent the assertion failure, this patch makes CheckDefaultArgumentVisitor visit all subexpressions belonging to a default argument expression and detect local variables and parameters (that are external to the default argument) referenced in the default argument expression after the template definition is parsed. This patch also removes the diagnostic that is printed in test p7.cpp when a local variable is referenced inside a unevaluated default argument expression, which I think conforms to c++14 or later.

Also, with this patch, clang prints diagnostics when local variables or parameters are referenced inside a block expression that is used as a default argument. I wasn't 100% sure it is legal to use blocks for default arguments (I found that compiling the code below causes clang to segfault), but it seems to me that we want to handle blocks in default arguments the same way we handle lambdas.

void logRange(id i = [](){}) {
}

void foo1() {
  logRange();
}

$ clang++ -std=c++14 -c -o /dev/null -fobjc-arc test.mm

rdar://problem/33239958

Diff Detail

Event Timeline

ahatanak created this revision.Aug 18 2017, 7:23 PM
faisalv edited edge metadata.Aug 22 2017, 6:22 PM

I don't think this approach is entirely correct for at least the following reasons:

  1. in the lambda case the machinery that diagnoses capture failures should be the machinery erroring on the lambda (when the parameter is odr-used)
  2. in the unevaluated case, once you disable the error, the instantiation machinery will fail to find the corresponding instantiated parmvardecl.

I think - in addition to allowing unevaluated uses of parmvardecls by tweaking the DefaultArgumentChecker, you would also need to add the instantiated mappings of the parameters, prior to instantiating the default argument (to avoid the assertion) and perhaps need to tweak DoMarkVarDeclReferenced and/or tryCaptureVariable to make sure such cases for lambdas produce errors (if they don't, w the above fix).

Thanks for working on this!

I don't think this approach is entirely correct for at least the following reasons:

  1. in the lambda case the machinery that diagnoses capture failures should be the machinery erroring on the lambda (when the parameter is odr-used)
  2. in the unevaluated case, once you disable the error, the instantiation machinery will fail to find the corresponding instantiated parmvardecl.

Oh right, it stills assert in the unevaluated case. I should have a test case for that too.

I also found that the following code, which has a lambda that doesn't capture anything, asserts (in IRGen):

struct S {
  template<class T>
  S(T&&) {}
};

template<class T>
void foo1() {
  struct S2 {
    void foo2(S s = [](){}) {
    }
  };

  S2 s2;
  s2.foo2();
}

void foo3() {
  foo1<int>();
}

I don't think this approach is entirely correct for at least the following reasons:

  1. in the lambda case the machinery that diagnoses capture failures should be the machinery erroring on the lambda (when the parameter is odr-used)

Does this mean that it is OK to have a parameter or local variable appear in a potentially-evaluated expression as long as it is not odr-used? I think this is slightly different from the standard, which says a parameter or local variable cannot appear as a potentially-evaluated expression in a default argument.

For example:

void foo2(int);

void func() {
  const int a = 1;
  void foo1(S s = [](){ foo2(a); }); // "a" is not in an unevaluated context and is not odr-used.
}

I think I need clarification as it will affect how or where we detect this error.

I don't think this approach is entirely correct for at least the following reasons:

  1. in the lambda case the machinery that diagnoses capture failures should be the machinery erroring on the lambda (when the parameter is odr-used)

Does this mean that it is OK to have a parameter or local variable appear in a potentially-evaluated expression as long as it is not odr-used? I think this is slightly different from the standard, which says a parameter or local variable cannot appear as a potentially-evaluated expression in a default argument.

For example:

void foo2(int);

void func() {
  const int a = 1;
  void foo1(S s = [](){ foo2(a); }); // "a" is not in an unevaluated context and is not odr-used.
}

I think I need clarification as it will affect how or where we detect this error.

'a' is not captured (and does not need to be captured) above, so I think that code should be ok.

But I also think the following code should be ok at local scope within func.
void foo(int = a);

I thought there was a DR against the standard-draft with the intent of making these valid (if they are not already so)?

clang currently rejects "void foo(int = a);" and so does gcc.

I'm trying to search the defect reports, but it looks like the c++ standard's site is currently unreachable.

ahatanak updated this revision to Diff 114753.Sep 11 2017, 7:29 PM

Address review comments.

  • Detect invalid references to parameters or local variables by default arguments in tryCaptureVariable. Before parsing or instantiating the default argument expression, the enclosing DeclContext is saved to ParentOfDefaultArg, which tryCaptureVariable uses to detect invalid references (if the referenced variable belongs to ParentOfDefaultArg or an enclosing DeclContext, it is not valid).
  • In CheckCXXDefaultArgExpr, save the parameters and their instantiations that appear before the parameter with default argument to the current LocalInstantiationScope so that findInstantiationOf doesn't assert when it tries to find the instantiation of a parameter that is referenced in the default arugment.

There are still cases where clang rejects references to local variables or parameters that shouldn't be rejected. For example:

  • Local variables or parameters referenced in _Generic's controlling-expression or the expressions of the selections that are not chosen.
  • The following code is rejected even though 'x' is not odr-used:
void func() {
  const int x = 1;
  void foo1(int a0 = x);
}
  • dcl.fct.default/p7.cpp

I plan to work on a fix after this patch is committed.

ping.

Any comments on this patch or alternate approaches?

This looks good to me.

aaron.ballman added inline comments.
lib/Sema/Sema.cpp
1677–1684

Any reason not to inline this in the header file?

lib/Sema/SemaExpr.cpp
4523

const FunctionDecl *

4535

Is it possible for the Optional<> returned here to not hold a value?

13746–13751

Ternary operator in S.Diag() would be just as clear, I think.

Also, you should not need to call getDeclName() as the diagnostic engine will automatically do the right thing. As is stands, this won't properly quote the name in the diagnostic.

Please take a look at http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0588r1.html, which respecifies the rules for lambda capture and its interaction with default arguments, and has been voted into the C++ working paper as a defect report resolution. The approach there is to use a purely syntactic, scope-based mechanism to detect problems such as this. (In dependent contexts, we can track on the DeclRefExpr whether the name is odr-usable, in case we can't tell whether it's odr-used from the template definition alone.)

include/clang/Sema/Sema.h
1062

There are lots of cases where we switch context in the middle of handling an expression, for instance to instantiate a template (or even *parse* a template in MSVC-compatible delayed template parsing mode). It's not reasonable to add a new form of state that all those places will need to save and restore themselves.

Please consider whether this would make sense as a member of the ExpressionEvaluationContextRecord or similar.

lib/Sema/SemaExpr.cpp
4520–4541

Use addInstantiatedParametersToScope for this.

This bugfix looks to be independent of the fix for lambdas; can you factor it out into a separate patch?