Page MenuHomePhabricator

[SemaTemplateInstantiate] Handle lack of TypeSourceInfo on special member functions in templated lambdas
ClosedPublic

Authored by bruno on Sep 25 2020, 11:47 AM.

Details

Summary

During template instantiation involving templated lambdas, clang could hit an assertion in TemplateDeclInstantiator::SubstFunctionType since the functions are not associated with any TypeSourceInfo:

assert(OldTInfo && "substituting function without type source info");

This path is triggered when using templated lambdas like the one added as a test to this patch. To fix this:

  • Create TypeSourceInfos for special members and make sure the template instantiator can get through all patterns.
  • Introduce a SpecialMemberTypeInfoRebuilder tree transform to rewrite such member function arguments. Without this, we get errors like:

error: only special member functions and comparison operators may be defaulted

since getDefaultedFunctionKind can't properly recognize these functions as special members as part of SetDeclDefaulted.

Fixes PR45828 and PR44848.

Diff Detail

Event Timeline

bruno created this revision.Sep 25 2020, 11:47 AM
bruno requested review of this revision.Sep 25 2020, 11:47 AM
bruno edited the summary of this revision. (Show Details)Sep 25 2020, 12:09 PM

I found this PR when investigating a compiler error that triggers the same exception and that can be tracked back to clang 3.5. First of all, thank you for looking into this.

I applied your path to 5b742a0c106fbed11779d6dd99854a6f97643524.

Here is your test case without explicit template lambdas. This, too, fails on trunk, but is fixed with your changes:

template <typename F>
void a(F f) {
  f.operator()(0);
}

template <typename F>
void b(F f) {
  a([=](auto i) {
    f.operator()(i);
  });
}

void c() {
  b([&](auto i) {
  });
}

This version, which is now C++14-conformant, can be compiled in c++20 and c++17 modes, but continues to fail with c++14 with the same assertion triggering.

Here is a much shorter example that shows the same behavior (broken everywhere on trunk, fixed by your changes for c++17 and 20, still broken for c++14):

void kill() {
  const auto lambda = [&](auto arg1) {};

  [&](auto arg2) {
    lambda.operator()(arg2);
  }(0);
}

Interestingly, in both cases the problem disappears if the lambda is called directly (i.e., without .operator()). In the case that brought me here, knowing this allowed me to replace lambda.operator()<true>(...) with lambda(std::true_type{}, ...) and circumventing the compiler bug.

Your patch also fixes https://bugs.llvm.org/show_bug.cgi?id=44848 for c++17 and upwards.

bruno added a comment.Nov 2 2020, 10:29 AM

@mdreseler Thanks for trying it out with other testcases, lemme take one more look and make it work for your c++14 version as well. @rsmith does the approach in this patch sounds reasonable?

Interestingly, in both cases the problem disappears if the lambda is called directly (i.e., without .operator()). In the case that brought me here, knowing this allowed me to replace lambda.operator()<true>(...) with lambda(std::true_type{}, ...) and circumventing the compiler bug.

I didn't know the workaround, interesting!

bruno updated this revision to Diff 346029.May 17 2021, 6:01 PM
bruno edited the summary of this revision. (Show Details)
bruno added a reviewer: aaron.ballman.

Make this work for c++14 as well, update testcase to cover all possibilities mentioned. @mdreseler thanks for bringing this up.

aaron.ballman added inline comments.May 18 2021, 12:30 PM
clang/lib/Sema/SemaDeclCXX.cpp
13011–13012
clang/lib/Sema/SemaTemplateInstantiate.cpp
2820

Under what circumstances could we get here where Pattern is not dependent?

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
2269–2271
4853–4856

What about a defaulted spaceship operator? Do we do the right thing there already?

4859–4861
bruno added a comment.May 18 2021, 5:57 PM

Thanks for the review @aaron.ballman!

clang/lib/Sema/SemaTemplateInstantiate.cpp
2820

Consider this code:

1 const auto lambda = [&](auto arg1) {};
2 [&](auto arg2) { lambda.operator()(arg2); }(0);

While parsing lambda.operator()(arg2) in line 1, clang calls to DiagnoseUseOfDecl, which will deduce / tree transform the CXXRecordDecl for the lambda in line 1. This record isn't dependent according to isDependentContext()'s requirements for CXXRecordDecl (and it doesn't seem to be missing anything?), since the implicit class doesn't have an associated ClassTemplateDecl nor the lambda and context are dependent. We do have a FunctionTemplateDecl as part of CXXRecordDecl (because of operator()), but that doesn't play a game here.

PerformDependentDiagnostics relies on isDependentContext(), which leads to an assertion after the TSI problem is fixed in other places in the patch, that's what initially prompted me to change this.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
4853–4856

That's a good question, I believe it's orthogonal here.

As you noticed, the current approach bails out for anything not a special member, and that's because it only tackles the testcases I was able to produce, I haven't been able to come up with spaceship related use case that triggers or needs this code path. So I rather not add any code for that right now. If you have one, lemme know! Anything in that direction would likely benefit from its own patch/revisions, so we can track possible regressions separately.

bruno updated this revision to Diff 346318.May 18 2021, 6:01 PM

Update patch after @aaron.ballman review!

Nothing else of substance from me, but I don't feel confident in giving final sign off on this. LG, but please wait until one of the other reviewers accepts as well.

clang/lib/Sema/SemaTemplateInstantiate.cpp
2820

Ah, thank you for the explanation!

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
2269–2271

No need for the parens around isa any longer (the || was removed).

4853–4856

I can't think of a specific test case I'm worried about, I was more just thinking "oh, here's another special member function you can write =default on.

bruno added a comment.May 20 2021, 2:54 PM

Nothing else of substance from me, but I don't feel confident in giving final sign off on this. LG, but please wait until one of the other reviewers accepts as well.

No problemo, thanks for the review!

bruno updated this revision to Diff 346865.May 20 2021, 3:00 PM

Update after last round of reviews.

@rsmith Is the approach reasonable or is there a better way to go about this?

aaron.ballman accepted this revision.Jun 22 2021, 4:36 AM

LGTM! I think we can address any concerns from @rsmith post commit at this point.

This revision is now accepted and ready to land.Jun 22 2021, 4:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2021, 5:28 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript