[ASTImporter] Fix structural eq of lambdas

[martong]

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.

[a_sidorin]

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]

clang/lib/AST/ASTStructuralEquivalence.cpp

1182 ↗

(On Diff #207545)

Thanks, good catch, I have added it.

[martong]

• Add check for isLambda()

[a_sidorin]

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]

• Add test which crashes in baseline

[martong]

In 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:

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]

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

[martong]

@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/ .

[martong]

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

cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp

static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
   // Check the prototypes.
   if (!::IsStructurallyEquivalent(Context,
                                   Method1->getType(), Method2->getType()))
     return false;
 
   return true;
 }
 
+/// Determine structural equivalence of two lambda classes.
+static bool
+IsStructurallyEquivalentLambdas(StructuralEquivalenceContext &Context,
+                                CXXRecordDecl *D1, CXXRecordDecl *D2) {
+  assert(D1->isLambda() && D2->isLambda() &&
+         "Must be called on lambda classes");
+  if (!IsStructurallyEquivalent(Context, D1->getLambdaCallOperator(),
+                                D2->getLambdaCallOperator()))
+    return false;
+
+  return true;
+}
+
 /// Determine structural equivalence of two records.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
                                      RecordDecl *D1, RecordDecl *D2) {
   if (D1->isUnion() != D2->isUnion()) {
     if (Context.Complain) {
       Context.Diag2(D2->getLocation(), Context.getApplicableDiagnostic(
                                            diag::err_odr_tag_type_inconsistent))
           << Context.ToCtx.getTypeDeclType(D2);
static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
 
   if (auto *D1CXX = dyn_cast<CXXRecordDecl>(D1)) {
     if (auto *D2CXX = dyn_cast<CXXRecordDecl>(D2)) {
       if (D1CXX->hasExternalLexicalStorage() &&
           !D1CXX->isCompleteDefinition()) {
         D1CXX->getASTContext().getExternalSource()->CompleteType(D1CXX);
       }
 
+      if (D1CXX->isLambda() != D2CXX->isLambda())
+        return false;
+      if (D1CXX->isLambda()) {
+        if (!IsStructurallyEquivalentLambdas(Context, D1CXX, D2CXX))
+          return false;
+      }
+
       if (D1CXX->getNumBases() != D2CXX->getNumBases()) {
         if (Context.Complain) {
           Context.Diag2(D2->getLocation(),
                         Context.getApplicableDiagnostic(
                             diag::err_odr_tag_type_inconsistent))
               << Context.ToCtx.getTypeDeclType(D2);
           Context.Diag2(D2->getLocation(), diag::note_odr_number_of_bases)
               << D2CXX->getNumBases();

cfe/trunk/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionParam) {
   auto ToL = Import(FromL, Lang_CXX11);
   unsigned ToLSize = std::distance(ToL->decls().begin(), ToL->decls().end());
   unsigned FromLSize =
       std::distance(FromL->decls().begin(), FromL->decls().end());
   EXPECT_NE(ToLSize, 0u);
   EXPECT_EQ(ToLSize, FromLSize);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, LambdaInGlobalScope) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      auto l1 = [](unsigned lp) { return 1; };
+      auto l2 = [](int lp) { return 2; };
+      int f(int p) {
+        return l1(p) + l2(p);
+      }
+      )",
+      Lang_CXX11, "input0.cc");
+  FunctionDecl *FromF = FirstDeclMatcher<FunctionDecl>().match(
+      FromTU, functionDecl(hasName("f")));
+  FunctionDecl *ToF = Import(FromF, Lang_CXX11);
+  EXPECT_TRUE(ToF);
+}
+
 struct LLDBLookupTest : ASTImporterOptionSpecificTestBase {
   LLDBLookupTest() {
     Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
                  ASTContext &FromContext, FileManager &FromFileManager,
                  bool MinimalImport,
                  const std::shared_ptr<ASTImporterSharedState> &SharedState) {
       return new ASTImporter(ToContext, ToFileManager, FromContext,
                              FromFileManager, MinimalImport,

cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp

 }
 
 TEST_F(StructuralEquivalenceRecordTest, RecordsWithDifferentBody) {
   auto t = makeNamedDecls("class foo { int B; };", "class foo { int A; };",
                           Lang_CXX11);
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+struct StructuralEquivalenceLambdaTest : StructuralEquivalenceTest {};
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithDifferentMethods) {
+  // Get the LambdaExprs, unfortunately we can't match directly the underlying
+  // implicit CXXRecordDecl of the Lambda classes.
+  auto t = makeDecls<LambdaExpr>(
+      "void f() { auto L0 = [](int){}; }",
+      "void f() { auto L1 = [](){}; }",
+      Lang_CXX11,
+      lambdaExpr(),
+      lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_FALSE(testStructuralMatch(L0, L1));
+}
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithEqMethods) {
+  auto t = makeDecls<LambdaExpr>(
+      "void f() { auto L0 = [](int){}; }",
+      "void f() { auto L1 = [](int){}; }",
+      Lang_CXX11,
+      lambdaExpr(),
+      lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_TRUE(testStructuralMatch(L0, L1));
+}
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithDifferentFields) {
+  auto t = makeDecls<LambdaExpr>(
+      "void f() { char* X; auto L0 = [X](){}; }",
+      "void f() { float X; auto L1 = [X](){}; }",
+      Lang_CXX11,
+      lambdaExpr(),
+      lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_FALSE(testStructuralMatch(L0, L1));
+}
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithEqFields) {
+  auto t = makeDecls<LambdaExpr>(
+      "void f() { float X; auto L0 = [X](){}; }",
+      "void f() { float X; auto L1 = [X](){}; }",
+      Lang_CXX11,
+      lambdaExpr(),
+      lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_TRUE(testStructuralMatch(L0, L1));
+}
+
 TEST_F(StructuralEquivalenceTest, CompareSameDeclWithMultiple) {
   auto t = makeNamedDecls(
       "struct A{ }; struct B{ }; void foo(A a, A b);",
       "struct A{ }; struct B{ }; void foo(A a, B b);",
       Lang_CXX);
   EXPECT_FALSE(testStructuralMatch(t));
 }