[PATCH] D64075: [ASTImporter] Fix structural eq of lambdas

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D64075#1572676 <https://reviews.llvm.org/D64075#1572676>, @a_sidorin wrote:

> Hi Gabor,
>  The patch looks good, but it looks to me that it has a relation to https://reviews.llvm.org/D64078 that is kind of questionable to me. Let's delay landing this patch until the fix direction is clear.


Hi Alexey,

I have added a test case, which demonstrates the problem. When we import lambdas in a global/namespace scope then we crash. Let's try to import `f` from below:

  auto l1 = [](unsigned lp) { return 1; };
  auto l2 = [](int lp) { return 2; };
  int f(int p) {
    return l1(p) + l2(p);
  }

Now during the import of the second lambda expression we crash because during the import of the second lambda class we find the existing lambda class and we just merge into that the new `operator()` and this results having two `operator()`s in the lambda class:

  ASTTests: ../../git/llvm-project/clang/lib/AST/DeclCXX.cpp:1392: clang::CXXMethodDecl *clang::CXXRecordDecl::getLambdaCallOperator() const: Assertion `allLookupResultsAreTheSame(Calls) && "More than one lambda call operator!"' failed.
    #0 0x00007f4d2b838d39 llvm::sys::PrintStackTrace(llvm::raw_ostream&) /home/egbomrt/WORK/llvm5/build/debug/../../git/llvm-project/llvm/lib/Support/Unix/Signals.inc:494:11
    #1 0x00007f4d2b838ee9 PrintStackTraceSignalHandler(void*) /home/egbomrt/WORK/llvm5/build/debug/../../git/llvm-project/llvm/lib/Support/Unix/Signals.inc:558:1
    #2 0x00007f4d2b8377d6 llvm::sys::RunSignalHandlers() /home/egbomrt/WORK/llvm5/build/debug/../../git/llvm-project/llvm/lib/Support/Signals.cpp:67:5
    #3 0x00007f4d2b83955b SignalHandler(int) /home/egbomrt/WORK/llvm5/build/debug/../../git/llvm-project/llvm/lib/Support/Unix/Signals.inc:357:1
    #4 0x00007f4d2b351390 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x11390)
    #5 0x00007f4d283d6428 gsignal /build/glibc-LK5gWL/glibc-2.23/signal/../sysdeps/unix/sysv/linux/raise.c:54:0
    #6 0x00007f4d283d802a abort /build/glibc-LK5gWL/glibc-2.23/stdlib/abort.c:91:0
    #7 0x00007f4d283cebd7 __assert_fail_base /build/glibc-LK5gWL/glibc-2.23/assert/assert.c:92:0
    #8 0x00007f4d283cec82 (/lib/x86_64-linux-gnu/libc.so.6+0x2dc82)
    #9 0x00007f4d2ab94b00 clang::CXXRecordDecl::getLambdaCallOperator() const /tmp/../../git/llvm-project/clang/lib/AST/DeclCXX.cpp:0:107
   #10 0x00007f4d2acd3752 clang::LambdaExpr::getCallOperator() const /home/egbomrt/WORK/llvm5/build/debug/../../git/llvm-project/clang/lib/AST/ExprCXX.cpp:1205:3
   #11 0x00007f4d2acd36c3 clang::LambdaExpr::LambdaExpr(clang::QualType, clang::SourceRange, clang::LambdaCaptureDefault, clang::SourceLocation, llvm::ArrayRef<clang::LambdaCapture>, bool, bool, llvm::ArrayRef<clang::Expr*>, clang::SourceLocation, bool) /home/egbomrt/WORK/llvm5/build/debug/../../git/llvm-project/clang/lib/AST/ExprCXX.cpp:1128:34
   #12 0x00007f4d2acd394f clang::LambdaExpr::Create(clang::ASTContext const&, clang::CXXRecordDecl*, clang::SourceRange, clang::LambdaCaptureDefault, clang::SourceLocation, llvm::ArrayRef<clang::LambdaCapture>, bool, bool, llvm::ArrayRef<clang::Expr*>, clang::SourceLocation, bool) /home/egbomrt/WORK/llvm5/build/debug/../../git/llvm-project/clang/lib/AST/ExprCXX.cpp:1143:10
   #13 0x00007f4d2a90b078 clang::ASTNodeImporter::VisitLambdaExpr(clang::LambdaExpr*) /tmp/../../git/llvm-project/clang/lib/AST/ASTImporter.cpp:7461:10

Now if we slightly change the test code to make the lambdas op() have the very same signature then we end up with the problem we want to solve in D64078 <https://reviews.llvm.org/D64078>:

  auto l1 = [](int lp) { return 1; };
  auto l2 = [](int lp) { return 2; };
  int f(int p) {
    return l1(p) + l2(p);
  }

There is no crash if the lambdas' decl context is a function or method, this is because in that case we simply skip the lookup, in `VisitRecordDecl` we have

  if (!DC->isFunctionOrMethod()) {
    // lookup is done here!
  }

An alternative solution might be to disable lookup completely if the decl in the "from" context is a lambda. However, that would have other problems: what if the lambda is defined in a header file and included in several TUs? (I think we'd have as many duplicates as many includes we have, but we could live with that maybe). Would you prefer this alternative solution?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64075/new/

https://reviews.llvm.org/D64075