Page MenuHomePhabricator

[ASTImporter] Fix structural eq of lambdas
ClosedPublic

Authored by martong on Jul 2 2019, 6:26 AM.

Details

Summary

The structural equivalence check reported false eq between lambda classes
with different parameters in their call signature.
The solution is to check the methods for equality too in case of lambda
classes.

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.Jul 2 2019, 6:26 AM

Hello Gabor,
This looks mostly good but I have a question inline.

clang/lib/AST/ASTStructuralEquivalence.cpp
1182 ↗(On Diff #207545)

Should we return false if D1CXX->isLambda() != D2CXX->isLambda()?

martong marked 2 inline comments as done.Jul 3 2019, 6:17 AM
martong added inline comments.
clang/lib/AST/ASTStructuralEquivalence.cpp
1182 ↗(On Diff #207545)

Thanks, good catch, I have added it.

martong updated this revision to Diff 207774.Jul 3 2019, 6:17 AM
martong marked an inline comment as done.
  • Add check for isLambda()

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.

martong updated this revision to Diff 208727.Jul 9 2019, 9:58 AM
  • Add test which crashes in baseline

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:

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?

a_sidorin accepted this revision.Jul 14 2019, 11:11 AM

Hello Gabor,
This patch looks good to me.
Regarding the related patch: can we use getLambdaManglingNumber() for the comparison?

This revision is now accepted and ready to land.Jul 14 2019, 11:11 AM

@shafik I've been looking for any lldb regression in our Mac machine, could not find any. Now I am looking at http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/ .

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2019, 7:40 AM

Jenkins looks okay: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/31160/ .The build is red but the the previous run has the very same failing test case:
expression_command/weak_symbols/TestWeakSymbols.py