[modules] Delay calling DeclMustBeEmitted until it's safe

[teemperor]

Summary

This patch implements the suggestion in D29753 that calling DeclMustBeEmitted in the middle of deserialization should be avoided and that the actual check should be deferred until it's safe to do so.

This patch fixes the attached test case we encountered when compiling our code with modules: Here clang would crash when accessing the invalid redecl chains while trying to evaluate the value of a const VarDecl that contains a function call. I've also added a test case for the the assertion fail hat Richard mentioned in D29753 which is also fixed by this patch.

[rsmith]

Can you also add the test from PR32186?

include/clang/Serialization/ASTReader.h

987–1001

Too much indentation here.

995

Please lowercase the first letter of this function.

lib/Serialization/ASTReaderDecl.cpp

3634

Too much indentation.

3648

I'm concerned about this call; loadDeclUpdateRecords is called when the AST invariants are not met, so it's not safe to call DeclMustBeEmitted from here in general.

There are two ways we can get here: either we just loaded a declaration and we're updating it (in which case we can entirely skip both this check and adding an entry to PotentiallyInterestingDecls, because there already is one), or it's a pre-existing declaration and we've just loaded a new module file (in which case I think it's safe to call DeclMustBeEmitted from here). We could track which case we're in on PendingUpdateRecords and conditionally skip this.

3668

This call looks problematic too. Can we just remove it? We're going to do the same check again later anyway.

[teemperor]

• Renamed InterestingDecls to PotentiallyInterestingDecls to better reflect its purpose.
• Ran patch through clang-format (now even with the correct code-style) and did the other requested style fixes.
• Added test from PR32186.

[bruno]

@teemperor, can you rename PR32186 to something more meaningful?

[teemperor]

lib/Serialization/ASTReaderDecl.cpp

3668

The purpose seems to be that we only send every declaration once to the consumer.

All initially interesting decls were already in the list from the start and we now add all decls that became interesting since then. Without this check we send some declarations multiple times to the consumer and it seems to me that this wasn't intended (even though no test case in clang actually seems to test against this behavior)

What we could do is make the final list of declarations value-unique before passing it o the consumer and then remove those two checks above?

[teemperor]

• Prefixed PR31863.cpp with something meaningful.
• Added comment to delaly-emit-check.cpp to explain what we test there.

[rsmith]

lib/Serialization/ASTReaderDecl.cpp

3649

Why is this safe to call now? If we just loaded D, this could trigger deserializations and generally have the same problems as other calls to this function.

[teemperor]

• Removed the work-in-progress changes to: loadDeclUpdateRecords
• Added FIXME that this function should be patched too.

Our code is fixed with this patch and fixing the unsafe calls in loadDeclUpdateRecords seems like a more complicated problem. I would say this is out of scope for this patch. I'll make another patch that addresses loadDeclUpdateRecords and keep this patch just for fixing the single isConsumerInterestedIn call in ReadDeclRecord.

[teemperor]

• Fixed compilation error in the cleaned up version (removed two lines too much...)

[rsmith]

This is not a complete fix, since the call within loadDeclUpdateRecords could still be problematic, but it's at least an improvement.

Also, phabricator is not showing any test changes with the latest version of the patch. I assume they're still there and unchanged from the previous version.

lib/Serialization/ASTReaderDecl.cpp

3676

Delete this call to isConsumerInterestedIn -- there's no point calling it here given that PassInterestingDeclsToConsumer is going to do the same check.

[teemperor]

• Readded the missing tests to get the final diff.

[v.g.vassilev]

Landed in r300110.

include/clang/Serialization/ASTReader.h

private:
   /// \brief The set of lookup results that we have faked in order to support
   /// merging of partially deserialized decls but that we have not yet removed.
   llvm::SmallMapVector<IdentifierInfo *, SmallVector<NamedDecl*, 2>, 16>
     PendingFakeLookupResults;
 
   /// \brief The generation number of each identifier, which keeps track of
   /// the last time we loaded information about this identifier.
   llvm::DenseMap<IdentifierInfo *, unsigned> IdentifierGeneration;
-  
-  /// \brief Contains declarations and definitions that will be
+  class InterestingDecl {
+    Decl *D;
+    bool DeclHasPendingBody;
+
+  public:
+    InterestingDecl(Decl *D, bool HasBody)
+        : D(D), DeclHasPendingBody(HasBody) {}
+    Decl *getDecl() { return D; }
+    /// Whether the declaration has a pending body.
+    bool hasPendingBody() { return DeclHasPendingBody; }
+  };
+
+  /// \brief Contains declarations and definitions that could be
   /// "interesting" to the ASTConsumer, when we get that AST consumer.
   ///
   /// "Interesting" declarations are those that have data that may
   /// need to be emitted, such as inline function definitions or
   /// Objective-C protocols.
-  std::deque<Decl *> InterestingDecls;
+  std::deque<InterestingDecl> PotentiallyInterestingDecls;
 
   /// \brief The list of redeclaration chains that still need to be 
   /// reconstructed, and the local offset to the corresponding list
   /// of redeclarations.
   SmallVector<std::pair<Decl *, uint64_t>, 16> PendingDeclChains;
 
   /// \brief The list of canonical declarations whose redeclaration chains
   /// need to be marked as incomplete once we're done deserializing things.

lib/Serialization/ASTReader.cpp

   assert(ImplD && Consumer);
 
   for (auto *I : ImplD->methods())
     Consumer->HandleInterestingDecl(DeclGroupRef(I));
 
   Consumer->HandleInterestingDecl(DeclGroupRef(ImplD));
 }
 
-void ASTReader::PassInterestingDeclsToConsumer() {
-  assert(Consumer);
-
-  if (PassingDeclsToConsumer)
-    return;
-
-  // Guard variable to avoid recursively redoing the process of passing
-  // decls to consumer.
-  SaveAndRestore<bool> GuardPassingDeclsToConsumer(PassingDeclsToConsumer,
-                                                   true);
-
-  // Ensure that we've loaded all potentially-interesting declarations
-  // that need to be eagerly loaded.
-  for (auto ID : EagerlyDeserializedDecls)
-    GetDecl(ID);
-  EagerlyDeserializedDecls.clear();
-
-  while (!InterestingDecls.empty()) {
-    Decl *D = InterestingDecls.front();
-    InterestingDecls.pop_front();
-
-    PassInterestingDeclToConsumer(D);
-  }
-}
-
 void ASTReader::PassInterestingDeclToConsumer(Decl *D) {
   if (ObjCImplDecl *ImplD = dyn_cast<ObjCImplDecl>(D))
     PassObjCImplDeclToConsumer(ImplD, Consumer);
   else
     Consumer->HandleInterestingDecl(DeclGroupRef(D));
 }
 
 void ASTReader::StartTranslationUnit(ASTConsumer *Consumer) {

lib/Serialization/ASTReaderDecl.cpp

     if (Class->isThisDeclarationADefinition() ||
         PendingDefinitions.count(Class))
       loadObjCCategories(ID, Class);
   
   // If we have deserialized a declaration that has a definition the
   // AST consumer might need to know about, queue it.
   // We don't pass it to the consumer immediately because we may be in recursive
   // loading, and some declarations may still be initializing.
-  if (isConsumerInterestedIn(Context, D, Reader.hasPendingBody()))
-    InterestingDecls.push_back(D);
+  PotentiallyInterestingDecls.push_back(
+      InterestingDecl(D, Reader.hasPendingBody()));
 
   return D;
 }
 
+void ASTReader::PassInterestingDeclsToConsumer() {
+  assert(Consumer);
+
+  if (PassingDeclsToConsumer)
+    return;
+
+  // Guard variable to avoid recursively redoing the process of passing
+  // decls to consumer.
+  SaveAndRestore<bool> GuardPassingDeclsToConsumer(PassingDeclsToConsumer,
+                                                   true);
+
+  // Ensure that we've loaded all potentially-interesting declarations
+  // that need to be eagerly loaded.
+  for (auto ID : EagerlyDeserializedDecls)
+    GetDecl(ID);
+  EagerlyDeserializedDecls.clear();
+
+  while (!PotentiallyInterestingDecls.empty()) {
+    InterestingDecl D = PotentiallyInterestingDecls.front();
+    PotentiallyInterestingDecls.pop_front();
+    if (isConsumerInterestedIn(Context, D.getDecl(), D.hasPendingBody()))
+      PassInterestingDeclToConsumer(D.getDecl());
+  }
+}
+
 void ASTReader::loadDeclUpdateRecords(serialization::DeclID ID, Decl *D) {
   // The declaration may have been modified by files later in the chain.
   // If this is the case, read the record containing the updates from each file
   // and pass it to ASTDeclReader to make the modifications.
   ProcessingUpdatesRAIIObj ProcessingUpdates(*this);
   DeclUpdateOffsetsMap::iterator UpdI = DeclUpdateOffsets.find(ID);
   if (UpdI != DeclUpdateOffsets.end()) {
     auto UpdateOffsets = std::move(UpdI->second);
     DeclUpdateOffsets.erase(UpdI);
 
+    // FIXME: This call to isConsumerInterestedIn is not safe because
+    // we could be deserializing declarations at the moment. We should
+    // delay calling this in the same way as done in D30793.
     bool WasInteresting = isConsumerInterestedIn(Context, D, false);
     for (auto &FileAndOffset : UpdateOffsets) {
       ModuleFile *F = FileAndOffset.first;
       uint64_t Offset = FileAndOffset.second;
       llvm::BitstreamCursor &Cursor = F->DeclsCursor;
       SavedStreamPosition SavedPosition(Cursor);
       Cursor.JumpToBit(Offset);
       unsigned Code = Cursor.ReadCode();
       ASTRecordReader Record(*this, *F);
       unsigned RecCode = Record.readRecord(Cursor, Code);
       (void)RecCode;
       assert(RecCode == DECL_UPDATES && "Expected DECL_UPDATES record!");
 
       ASTDeclReader Reader(*this, Record, RecordLocation(F, Offset), ID,
                            SourceLocation());
       Reader.UpdateDecl(D);
 
       // We might have made this declaration interesting. If so, remember that
       // we need to hand it off to the consumer.
       if (!WasInteresting &&
           isConsumerInterestedIn(Context, D, Reader.hasPendingBody())) {
-        InterestingDecls.push_back(D);
+        PotentiallyInterestingDecls.push_back(
+            InterestingDecl(D, Reader.hasPendingBody()));
         WasInteresting = true;
       }
     }
   }
 
   // Load the pending visible updates for this decl context, if it has any.
   auto I = PendingVisibleUpdates.find(ID);
   if (I != PendingVisibleUpdates.end()) {

test/Modules/Inputs/PR32186/a.h

+#pragma once
+
+template <class T>
+struct base {
+  inline static constexpr T max() noexcept { return 10; }
+};
+
+template <typename T>
+static const constexpr T highest = base<T>::max();
+
+template <bool v>
+using cond_t = int;
+
+template <unsigned N>
+using dep_t = cond_t<(N < highest<int>)>;
+
+template <unsigned N>
+struct foo {
+  using type = dep_t<N>;
+};

test/Modules/Inputs/PR32186/module.modulemap

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

test/Modules/Inputs/delay-emit-check/Module.h

+namespace a {
+namespace b {
+int foo();
+const int x = foo();
+}
+}

test/Modules/Inputs/delay-emit-check/module.modulemap

+module "a" { header "Module.h" }

test/Modules/delay-emit-check-PR32186.cpp

+// RUN: %clang_cc1 -x c++ -fmodules-cache-path=%t -std=c++1z -fmodules -I%S/Inputs/PR32186/ -fmodule-map-file=%S/Inputs/PR32186/module.modulemap %s
+
+#include <a.h>
+
+template struct foo<10>;
+int main() { return 0; }

test/Modules/delay-emit-check.cpp

+// RUN: %clang_cc1 -fmodules-cache-path=%t/cache -fmodules -fimplicit-module-maps -emit-obj -I%S/Inputs/delay-emit-check/ %s -o %t.o
+
+// Test that we don't make unsafe calls such as to DeclMustBeEmitted during
+// deserialization.
+
+namespace a {
+namespace b {
+int foo();
+}
+}
+#include "Module.h"

test/PCH/Inputs/empty-def-fwd-struct.modulemap

+module M { header "../empty-def-fwd-struct.h" }
+

test/PCH/delay-PCH-emit-check.cpp

+// RUN: %clang_cc1 -x c++ -std=c++14 -fmodules -emit-module -fmodule-name=M %S/Inputs/empty-def-fwd-struct.modulemap -o %t.pcm
+// RUN: %clang_cc1 -x c++ -std=c++14 -fmodules -emit-llvm -fmodule-file=%t.pcm %s -o %t.o
+
+#include "empty-def-fwd-struct.h"