lambdas in modules: make PendingFakeDefinitionData a set

[elsteveogrande]

Summary

Of the three enums, only two are used (Fake and FakeLoaded), and even when switching the mapped value to FakeLoaded, it's leading to an unexpected state anyway (i.e. there's no check for only Fake's in the map). Discard the enum, make this a simple set to test for membership / emptiness.

Test Plan: ninja check-clang-modules

[rsmith]

lib/Serialization/ASTReaderDecl.cpp

1766

Did you mean to add this FIXME?

4246

This does not appear to be NFC: the FakeLoaded vs absent-from-map distinction matters here. I think the idea here is that we still need to load the lexical contents of the declaration of the class that we pick to be the definition even if we've already found and merged another definition into it.

That said, if this is necessary, we should have some test coverage for it, and based on this change not breaking any of our tests, we evidently don't. :( I'll try this change out against our codebase and see if I can find a testcase.

[elsteveogrande]

lib/Serialization/ASTReaderDecl.cpp

1766

Yup: to indicate this is assert about faked up lambda definition will be addressed in an upcoming diff, i.e., fixing the lambda issues :) See D50948, D50949

4246

Thanks for checking this! Yup I missed this; count would be different and therefore this changes logic, which surprisingly didn't break things as far as I could see. I'll change this back to Fake vs. FakeLoaded.

[elsteveogrande]

lib/Serialization/ASTReaderDecl.cpp

1760–1765

(2) the find, and the flip to FakeLoaded

3080–3081

(1) the insert

4246

Hmm. In the lambda case, the FakeLoaded remains in the table, and we get "faked up a class definition but never saw the real one":

assert(PendingFakeDefinitionData.empty() &&
       "faked up a class definition but never saw the real one");

Failing assert here: lib/Serialization/ASTReader.cpp

So I'm digging into why PendingFakeDefinitionData isn't emptying out. Recapping the logic in this class (to make sure I know what's going on here / to help explain it to myself)

1. a Fake insert happens around 3081, after creating a fake/empty DefinitionData and setting on a new CXXRecordDecl (and its canonical decl if different)
2. this table is checked in MergeDefinitionData from 1765; if there's a Fake entry, flip it to FakeLoaded
3. the only removal that I could find is when a decl is updated with UPD_CXX_INSTANTIATED_CLASS_DEFINITION around line 4256, only if there wasn't an update and there's a lexical DC. (In any event the UPD_CXX_INSTANTIATED_... wouldn't fire in my case)

4255

(3) ... and finally the erase

[elsteveogrande]

I'll drop this one, since this is not the logic change we want.

include/clang/Serialization/ASTReader.h

PendingUpdateRecord(serialization::GlobalDeclID ID, Decl *D,
                         bool JustLoaded)
         : D(D), ID(ID), JustLoaded(JustLoaded) {}
   };
 
   /// Declaration updates for already-loaded declarations that we need
   /// to apply once we finish processing an import.
   llvm::SmallVector<PendingUpdateRecord, 16> PendingUpdateRecords;
 
-  enum class PendingFakeDefinitionKind { NotFake, Fake, FakeLoaded };
-
   /// The DefinitionData pointers that we faked up for class definitions
   /// that we needed but hadn't loaded yet.
-  llvm::DenseMap<void *, PendingFakeDefinitionKind> PendingFakeDefinitionData;
+  llvm::DenseSet<void *> PendingFakeDefinitionData;
 
   /// Exception specification updates that have been loaded but not yet
   /// propagated across the relevant redeclaration chain. The map key is the
   /// canonical declaration (used only for deduplication) and the value is a
   /// declaration that has an exception specification.
   llvm::SmallMapVector<Decl *, FunctionDecl *, 4> PendingExceptionSpecUpdates;
 
   /// Deduced return type updates that have been loaded but not yet propagated

lib/Serialization/ASTReaderDecl.cpp

if (DD.Definition != MergeDD.Definition) {
     Reader.PendingDefinitions.erase(MergeDD.Definition);
     MergeDD.Definition->setCompleteDefinition(false);
     Reader.mergeDefinitionVisibility(DD.Definition, MergeDD.Definition);
     assert(Reader.Lookups.find(MergeDD.Definition) == Reader.Lookups.end() &&
            "already loaded pending lookups for merged definition");
   }
 
   auto PFDI = Reader.PendingFakeDefinitionData.find(&DD);
-  if (PFDI != Reader.PendingFakeDefinitionData.end() &&
-      PFDI->second == ASTReader::PendingFakeDefinitionKind::Fake) {
+  if (PFDI != Reader.PendingFakeDefinitionData.end()) {
     // We faked up this definition data because we found a class for which we'd
     // not yet loaded the definition. Replace it with the real thing now.
+
+    Reader.PendingFakeDefinitionData.erase(PFDI);
+
+    // FIXME: handle serialized lambdas
     assert(!DD.IsLambda && !MergeDD.IsLambda && "faked up lambda definition?");
-    PFDI->second = ASTReader::PendingFakeDefinitionKind::FakeLoaded;
 
     // Don't change which declaration is the definition; that is required
     // to be invariant once we select it.
     auto *Def = DD.Definition;
     DD = std::move(MergeDD);
     DD.Definition = Def;
     return;
   }
if (auto *RD = dyn_cast<CXXRecordDecl>(DC)) {
     // we load the update record.
     if (!DD) {
       DD = new (Reader.getContext()) struct CXXRecordDecl::DefinitionData(RD);
       RD->setCompleteDefinition(true);
       RD->DefinitionData = DD;
       RD->getCanonicalDecl()->DefinitionData = DD;
 
       // Track that we did this horrible thing so that we can fix it later.
-      Reader.PendingFakeDefinitionData.insert(
-          std::make_pair(DD, ASTReader::PendingFakeDefinitionKind::Fake));
+      Reader.PendingFakeDefinitionData.insert(DD);
     }
 
     return DD->Definition;
   }
 
   if (auto *ED = dyn_cast<EnumDecl>(DC))
     return ED->getASTContext().getLangOpts().CPlusPlus? ED->getDefinition()
                                                       : nullptr;
case UPD_CXX_ADDED_FUNCTION_DEFINITION: {
       break;
     }
 
     case UPD_CXX_INSTANTIATED_CLASS_DEFINITION: {
       auto *RD = cast<CXXRecordDecl>(D);
       auto *OldDD = RD->getCanonicalDecl()->DefinitionData;
       bool HadRealDefinition =
           OldDD && (OldDD->Definition != RD ||
                     !Reader.PendingFakeDefinitionData.count(OldDD));
       RD->setParamDestroyedInCallee(Record.readInt());
       RD->setArgPassingRestrictions(
           (RecordDecl::ArgPassingKind)Record.readInt());
       ReadCXXRecordDefinition(RD, /*Update*/true);
 
       // Visible update is handled separately.
       uint64_t LexicalOffset = ReadLocalOffset();
       if (!HadRealDefinition && LexicalOffset) {
         Record.readLexicalDeclContextStorage(LexicalOffset, RD);
         Reader.PendingFakeDefinitionData.erase(OldDD);
       }
 
       auto TSK = (TemplateSpecializationKind)Record.readInt();
       SourceLocation POI = ReadSourceLocation();
       if (MemberSpecializationInfo *MSInfo =
               RD->getMemberSpecializationInfo()) {
         MSInfo->setTemplateSpecializationKind(TSK);
         MSInfo->setPointOfInstantiation(POI);