This is an archive of the discontinued LLVM Phabricator instance.

Fix for PR27015 (variable template initialized with a generic lambda expression)
ClosedPublic

Authored by ahatanak on Apr 15 2016, 1:20 PM.

Details

Summary

I'm sending a WIP patch which fixes PR27015 to get an early feedback from the community.

This patch attempts to fix a crash which happens when a variable template is initialized with a generic lambda expression. Please see the example in the email I sent to cfe-dev:

http://lists.llvm.org/pipermail/cfe-dev/2016-April/048391.html

This patch makes changes to ensure the instantiated lambda class (which is the lambda class for fn<char> in the example) gets the right parent DeclContex (which is Decl::TranslationUnit in the example). After applying this patch, clang no longer crash compiling the example program. However, it still crashes when it compiles the following code:

$ cat test0.cpp
template <class> auto fn = [] {};
template <typename> void fn1() { fn<char>; }

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

Assertion failed: (isDependentContext() && "cannot iterate dependent diagnostics of non-dependent context"), function ddiags, file include/clang/AST/DependentDiagnostic.h, line 176.
...
10 clang-3.8 0x000000010b4afd18 clang::Sema::PerformDependentDiagnostics(clang::DeclContext const*, clang::MultiLevelTemplateArgumentList const&) + 40
11 clang-3.8 0x000000010b46ee1b (anonymous namespace)::TemplateInstantiator::transformedLocalDecl(clang::Decl*, clang::Decl*) + 251

It fails when DeclContext::ddiags() is called on the lambda class of the old lambda expression because it's not a dependent context.

Diff Detail

Event Timeline

ahatanak updated this revision to Diff 53936.Apr 15 2016, 1:20 PM
ahatanak retitled this revision from to Fix for PR27015 (variable template initialized with a generic lambda expression).
ahatanak updated this object.
ahatanak added a reviewer: rsmith.
ahatanak added a subscriber: cfe-commits.
rsmith added inline comments.Apr 15 2016, 1:51 PM
lib/Sema/SemaTemplateInstantiateDecl.cpp
3909–3917

Instead of the other changes, switch CurContext to the context of the variable template here:

Sema::ContextRAII SwitchContext(*this, Var->getDeclContext());
ahatanak updated this revision to Diff 54088.Apr 18 2016, 11:40 AM

Address Richard's review comments and add a test case.

The test case currently asserts when fn0 is instantiated. This doesn't happen if I give the template parameter a name.

template <class N> auto fn0 = [] {};

instead of

template <class> auto fn0 = [] {};

I can prevent the assert if I make changes in Sema::ActOnTypeParameter to call S->AddDecl(Param) regardless of whether Param has a name. I'm not sure this is the right fix at the moment.

I'm looking for a way to avoid the assert in Sema::PerformDependentDiagnostics that is fired when a template parameter doesn't have a name.

In order to avoid the assert, CXXRecordDecl::isDependentLambda() should return true for the old lambda class, and in order to do that, somehow I have to set KnownDependent to true in Sema::ActOnStartOfLambdaDefinition when the lambda is used to initialize a variable template.

I can think of two ways to fix this:

  1. Make changes in Sema::ActOnTypeParameter to call S->AddDecl(Param) regardless of whether Param has a name. This looks harmless to me because the list of Scope's decls is used just to see if a declaration belongs to a certain scope.
  1. Add a field to Scope that indicates whether a variable template is being parsed.

Does this sound like a good idea or are there other ways to do what I'm trying to do?

ahatanak updated this revision to Diff 55356.Apr 27 2016, 5:01 PM

Made a couple of changes to distinguish between an explicit specialization ('template<>') and a template declaration which doesn't have a named template parameter (see the variable template fn0 in test case vartemplate-lambda.cpp for an example).

  • In Parser::ParseTemplateDeclarationOrSpecialization, clear the TemplateParamScope bit of the scope's flag if it's seen only explicit specializations.
  • In Sema::ActOnStartOfLambdaDefinition, instead of checking whether the decl list is empty, check the TemplateParamScope bit of the template scope to see if it's in a "real" template declaration scope. I initially tried just removing the decls_empty() check, but found out that it asserted when the following code in test/CodeGenCXX/mangle-lambdas.cpp was compiled:

template<> double StaticMembers<double>::z = []{return 42; }();

Also, I had to change the check in test/CXX/temp/temp.arg/temp.arg.nontype/p1.cpp. Because the dependent bit of the lambda class is now set to true, Sema::CheckTemplateArgument returns early (near line 4876) before printing the last two of the three diagnostic messages:

  1. error: lambda expression in an unevaluated operand
  2. error: non-type template argument is not a constant expression
  3. note: non-literal type 'const lambdas::(lambda at p1.cpp:2:21)' cannot be used in a constant expression

I'm not sure whether the lambda in p1.cpp should be considered dependent, but I don't think we've been doing the check correctly. If we add another template parameter in front of "I", clang prints only the first message:

template<int J, int I = ([] { return 5; }())>
int f();

rsmith added inline comments.Apr 27 2016, 5:42 PM
lib/Parse/ParseTemplate.cpp
150–152

Use ParseScopeFlags to change the flags here rather than calling setFlags directly. Please also use getFlags() & ~Scope::TemplateParamScope rather than ^ to make it more obvious that you're clearing the flag not just flipping it.

lib/Sema/SemaLambda.cpp
816–817

This should not be necessary. It looks like Scope::setFlags fails to update TemplateParamParent (etc) if the relevant flag changes. Instead of adding this if, try changing the body of Scope::setFlags to call Init(AnyParent, F);.

ahatanak updated this revision to Diff 55453.Apr 28 2016, 11:59 AM

Use ParseScopeFlags to clear the TemplateParamScope rather than calling setFlags directly.

I tried calling Init(AnyParent, F) is Scope::setFlags, but it caused a lot of test cases to fail, which I'm currently investigating.

If I try calling Init(AnyParent, F) in Scope::setFlags, clang fails to compile the following code because it cannot find the definition of struct "foo":

void foo3(void) 
  struct foo {
    int a, f;
  };
  char *np = nullptr;
  int *ip = &(((struct foo *)np)->f);
  *ip = 0;
}

I can fix this test if I add a boolean flag to Scope::Init, which tells the function to return early before DeclsInScope and other fields are cleared.

All the other tests pass too except for test/CXX/drs/dr1xx.cpp:

namespace dr183 { // dr183: sup 382
  template<typename T> struct A {};
  template<typename T> struct B {
    typedef int X;
  };
  template<> struct A<int> {
    typename B<int>::X x;
  };
}

clang used to compile this code without any warnings or errors, but it now issues the following warning if -std=c++98 is on the command line:

dr1xx.cpp:7:5: warning: 'typename' occurs outside of a template [-Wc++11-extensions]

typename B<int>::X x;
^~~~~~~~~

1 warning generated.

Is clang correct to issue the warning in this case?

ahatanak updated this revision to Diff 55484.Apr 28 2016, 2:26 PM
  • Defined a private overload of Scope::setFlags which is called from Scope::setFlags and Scope::Init.
  • Stop checking TmplScope->isTemplateParamScope() in Sema::ActOnStartOfLambdaDefinition.
  • Fix test case test/CXX/drs/dr1xx.cpp to check an error message (not a warning) is printed when it's compiled with -std=c++98.
rsmith accepted this revision.Apr 28 2016, 2:32 PM
rsmith edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Apr 28 2016, 2:32 PM
This revision was automatically updated to reflect the committed changes.

Thanks for the review. I committed the patch in r267956 and r267975.

Do you think I should make setFlags(unsigned F) return early if F == Flags?