[Modules] Fix creating fake definition data for lambdas.

[vsapsai]

Summary

During reading C++ definition data for lambda we can access
CXXRecordDecl representing lambda before we finished reading the
definition data. This can happen by reading a captured variable which is
VarDecl, then reading its decl context which is CXXMethodDecl operator(),
then trying to merge redeclarable methods and accessing
enclosing CXXRecordDecl. The call stack looks roughly like

VisitCXXRecordDecl
  ReadCXXRecordDefinition
    VisitVarDecl
      VisitCXXMethodDecl
        mergeRedeclarable
          getPrimaryContextForMerging

If we add fake definition data at this point, later we'll hit the assertion

Assertion failed: (!DD.IsLambda && !MergeDD.IsLambda && "faked up lambda definition?"), function MergeDefinitionData, file clang/lib/Serialization/ASTReaderDecl.cpp, line 1675.

The fix is to check if we are still reading real definition data and to
avoid adding a fake one in this case. Fixes PR32556.

rdar://problem/37461072

[aprantl]

clang/lib/Serialization/ASTReaderDecl.cpp

2969 ↗

(On Diff #134991)

Perhaps add a comment explaining what's going on in this early exit?

[vsapsai]

clang/lib/Serialization/ASTReaderDecl.cpp

2969 ↗

(On Diff #134991)

While I was trying to explain this early exit, I realized one of my assumptions was incorrect. Now I'm not sure returning nullptr is the right fix, maybe assigning incomplete definition data like

D->DefinitionData = DD;
ReadCXXDefinitionData(*DD, D);

is better.

I am trying to come up with a test case showing different behaviour for these 2 different fixes. But so far nothing yet. If anybody can tell which one is correct: null context for merging or not-yet-populated definition data, that would be helpful.

[vsapsai]

clang/lib/Serialization/ASTReaderDecl.cpp

2969 ↗

(On Diff #134991)

Looks like both fix approaches should work as in ASTDeclReader::ReadCXXDefinitionData we read decl only for lambda. If I'm not mistaken, for lambdas merging doesn't really make sense, so no context for merging doesn't have repercussions.

[rsmith]

clang/lib/Serialization/ASTReaderDecl.cpp

2969 ↗

(On Diff #134991)

Merging for lambdas does make sense in the context of inline functions (where we can import two definitions of the same inline function from two different modules, and the definition can contain a lambda).

I think setting D->DefinitionData early would be the preferable solution. We should also disable the "all the bits must match" checks in MergeDefinitionData if one of the DefinitionData objects has IsBeingDefined set (or, ideally, defer those checks until recursive deserialization is complete).

[vsapsai]

• Set D->DefinitionData early.

We already call MergeDefinitionData after definition data deserialization is
complete. And I removed previously added IsBeingDefined, so left the "all the
bits must match" checks in MergeDefinitionData as-is.

As for testing, requirement for Canon->DefinitionData != DD check is already
enforced by existing tests.

[vsapsai]

Ping.

[rsmith]

clang/lib/Serialization/ASTReaderDecl.cpp

1795 ↗

(On Diff #135179)

It's a little odd that we'll temporarily have two different declarations that think they're the definition in this case, but I don't actually know of anything that'll go wrong as a result.

(This seems easy to avoid, though, by checking whether there already is a definition earlier.)

[vsapsai]

• Don't set to-be-deserialized definition data if there is another decl with definition data.

[vsapsai]

clang/lib/Serialization/ASTReaderDecl.cpp

1795 ↗

(On Diff #135179)

Uploaded a new patch. Is this what you had in mind by not having two different declarations that think they're the definition?

[rsmith]

clang/lib/Serialization/ASTReaderDecl.cpp

1790 ↗

(On Diff #137299)

I think it would make sense to "claim" the definition more eagerly here, by also updating Canon->DefinitionData in this case, prior to reading the definition data (and in the case where we already have a canonical definition, pointing D->DefinitionData at that eagerly).

[vsapsai]

• Claim the definition data more eagerly.

Not sure that added "different" in the existing comment is actually useful.
It makes sense to me but don't know about others.

[rsmith]

clang/lib/Serialization/ASTReaderDecl.cpp

1789 ↗

(On Diff #137801)

No braces around single-line if bodies, please.

1798 ↗

(On Diff #137801)

Canon->DefinitionData can't be null here, so the first half of this check is redundant. If you're concerned about that, you can add an assert that it's equal to D->DefinitionData (and that both are non-null).

1800 ↗

(On Diff #137801)

This store is dead.

[vsapsai]

clang/lib/Serialization/ASTReaderDecl.cpp

1811 ↗

(On Diff #137801)

This store seems to be dead too. Need to spend more time in debugger to make sure that the code works the way I think it works.

[vsapsai]

• Some more cleanup. NFC.

[vsapsai]

clang/lib/Serialization/ASTReaderDecl.cpp

1798 ↗

(On Diff #137801)

MergeDefinitionData already has assertion Canon->DefinitionData is not null so adding assertion here doesn't add safety. And I don't think it would improve readability or make it easier to understand the code.

[vsapsai]

I think we have agreement on this change. If there are no objections, I plan to commit it on Wednesday, March 21.

lib/Serialization/ASTReaderDecl.cpp

void ASTDeclReader::ReadCXXRecordDefinition(CXXRecordDecl *D, bool Update) {
   // allocate the appropriate DefinitionData structure.
   bool IsLambda = Record.readInt();
   if (IsLambda)
     DD = new (C) CXXRecordDecl::LambdaDefinitionData(D, nullptr, false, false,
                                                      LCD_None);
   else
     DD = new (C) struct CXXRecordDecl::DefinitionData(D);
 
+  CXXRecordDecl *Canon = D->getCanonicalDecl();
+  // Set decl definition data before reading it, so that during deserialization
+  // when we read CXXRecordDecl, it already has definition data and we don't
+  // set fake one.
+  if (!Canon->DefinitionData)
+    Canon->DefinitionData = DD;
+  D->DefinitionData = Canon->DefinitionData;
   ReadCXXDefinitionData(*DD, D);
 
-  // We might already have a definition for this record. This can happen either
-  // because we're reading an update record, or because we've already done some
-  // merging. Either way, just merge into it.
-  CXXRecordDecl *Canon = D->getCanonicalDecl();
-  if (Canon->DefinitionData) {
+  // We might already have a different definition for this record. This can
+  // happen either because we're reading an update record, or because we've
+  // already done some merging. Either way, just merge into it.
+  if (Canon->DefinitionData != DD) {
     MergeDefinitionData(Canon, std::move(*DD));
-    D->DefinitionData = Canon->DefinitionData;
     return;
   }
 
   // Mark this declaration as being a definition.
   D->IsCompleteDefinition = true;
-  D->DefinitionData = DD;
 
   // If this is not the first declaration or is an update record, we can have
   // other redeclarations already. Make a note that we need to propagate the
   // DefinitionData pointer onto them.
-  if (Update || Canon != D) {
-    Canon->DefinitionData = D->DefinitionData;
+  if (Update || Canon != D)
     Reader.PendingDefinitions.insert(D);
-  }
-}
 
 ASTDeclReader::RedeclarableResult
 ASTDeclReader::VisitCXXRecordDeclImpl(CXXRecordDecl *D) {
   RedeclarableResult Redecl = VisitRecordDeclImpl(D);
 
   ASTContext &C = Reader.getContext();
 
   enum CXXRecKind {

test/Modules/Inputs/self-referencing-lambda/a.h

+void f() {
+  int x = 0;
+  auto q = [xm = x]{};
+}

test/Modules/Inputs/self-referencing-lambda/module.modulemap

+module "a.h" {
+  header "a.h"
+  export *
+}

test/Modules/self-referencing-lambda.cpp

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/self-referencing-lambda %s -verify -emit-obj -o %t2.o
+// expected-no-diagnostics
+
+#include "a.h"