Fix for Bug 28332

[CrisCristescu]

Summary

The ASTWriter's mutation listener ignores mutations that come from AST files.

[rsmith]

I don't think this is right: we might load a FunctionDecl from an AST file, and then locally resolve its exception specification. In that case, we need to emit an update record.

[CrisCristescu]

Ok, I was trying to find the correct place to emit un update record, but I am not sure I have found it. I was looking at the Declare/Define ImplicitDestructor but there is an update record emitted there if the Function Decl comes from an ASTFile, but the Declare/Define does not seem to come from the AST file when it is created in our example.
On the other hand, if I replace the ~SmallVectorImpl() with the default constructor SmallVectorImpl() then the assertion is not reproducible. Thus, I tried to see what is different when Declare/DefineImplicitDefaultConstructor, but I could not see any difference in the code path in comparison to the Destructor.
So I believe SmallVectorImpl() is not implicitly defined and the update record should be emitted somewhere else. Any hint to where should I look?

[CrisCristescu]

This is a new suggestion as a result from the irc discussion.

The worry is about the setting and unsetting the ASTReader's flag: ProcessingUpdateRecords. The suggestion was that is should be set in ASTReader::loadDeclsUpdateRecords. Setting it there works, but if it is unset in the same load method then the callback to ResolveExceptionSpec is later then that and the flag is not set anymore.

Here is the backtrace of the assertion to:
clang: /home/biancacr/clang_build_/src/tools/clang/lib/Serialization/ASTWriter.cpp:5718: virtual void clang::ASTWriter::ResolvedExceptionSpec(const clang::FunctionDecl*): Assertion `!DoneWritingDeclsAndTypes && "Already done writing updates!"' failed.

Program received signal SIGABRT, Aborted.
[Switching to Thread 0x7ffff6980700 (LWP 12904)]
0x00007ffff69b61c7 in GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:55
55 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0 0x00007ffff69b61c7 in GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:55
#1 0x00007ffff69b7e2a in GI_abort () at abort.c:89
#2 0x00007ffff69af0bd in assert_fail_base (fmt=0x7ffff6b10f78 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",

assertion=assertion@entry=0x62fff88 "!DoneWritingDeclsAndTypes && \"Already done writing updates!\"", 
file=file@entry=0x62fcc00 "/home/biancacr/clang_build_/src/tools/clang/lib/Serialization/ASTWriter.cpp", line=line@entry=5718, 
function=function@entry=0x6334f80 <clang::ASTWriter::ResolvedExceptionSpec(clang::FunctionDecl const*)::__PRETTY_FUNCTION__> "virtual void clang::ASTWriter::ResolvedExceptionSpec(const clang::FunctionDecl*)") at assert.c:92

#3 0x00007ffff69af172 in GI_assert_fail (assertion=0x62fff88 "!DoneWritingDeclsAndTypes && \"Already done writing updates!\"",

file=0x62fcc00 "/home/biancacr/clang_build_/src/tools/clang/lib/Serialization/ASTWriter.cpp", line=5718, 
function=0x6334f80 <clang::ASTWriter::ResolvedExceptionSpec(clang::FunctionDecl const*)::__PRETTY_FUNCTION__> "virtual void clang::ASTWriter::ResolvedExceptionSpec(const clang::FunctionDecl*)")
at assert.c:101

#4 0x0000000003c9d75a in clang::ASTWriter::ResolvedExceptionSpec (this=0x7ffff001aae8, FD=0x7ffff0086b30) at /home/biancacr/clang_build_/src/tools/clang/lib/Serialization/ASTWriter.cpp:5718
#5 0x0000000002ceb1e9 in clang::MultiplexASTMutationListener::ResolvedExceptionSpec (this=0x7ffff0012030, FD=0x7ffff0086b30)

at /home/biancacr/clang_build_/src/tools/clang/lib/Frontend/MultiplexConsumer.cpp:177

#6 0x0000000003b7960c in clang::ASTReader::FinishedDeserializing (this=0x7ffff001b620) at /home/biancacr/clang_build_/src/tools/clang/lib/Serialization/ASTReader.cpp:8647
#7 0x0000000003b7ce85 in clang::ExternalASTSource::Deserializing::~Deserializing (this=0x7ffff697a670, __in_chrg=<optimized out>)

at /home/biancacr/clang_build_/src/tools/clang/include/clang/AST/ExternalASTSource.h:69

#8 0x0000000003b6a7d4 in clang::ASTReader::FindExternalVisibleDeclsByName (this=0x7ffff001b620, DC=0x7ffff0058068, Name=...)

at /home/biancacr/clang_build_/src/tools/clang/lib/Serialization/ASTReader.cpp:6667

#9 0x0000000004b69d6d in clang::DeclContext::lookup (this=0x7ffff0058068, Name=...) at /home/biancacr/clang_build_/src/tools/clang/lib/AST/DeclBase.cpp:1443
#10 0x0000000003c89c0c in clang::ASTWriter::GenerateNameLookupTable (this=0x7ffff001aae8, ConstDC=0x7ffff0058068, LookupTable=...)

and here is the backtrace of where loadDeclsUpdateRecords is called:
#0 clang::ASTReader::loadDeclUpdateRecords (this=0x7ffff001b620, ID=1, D=0x7ffff0019940) at /home/biancacr/clang_build_/src/tools/clang/lib/Serialization/ASTReaderDecl.cpp:3536
#1 0x0000000003b77e3e in clang::ASTReader::finishPendingActions (this=0x7ffff001b620) at /home/biancacr/clang_build_/src/tools/clang/lib/Serialization/ASTReader.cpp:8386
#2 0x0000000003b79441 in clang::ASTReader::FinishedDeserializing (this=0x7ffff001b620) at /home/biancacr/clang_build_/src/tools/clang/lib/Serialization/ASTReader.cpp:8634
#3 0x0000000003b7ce85 in clang::ExternalASTSource::Deserializing::~Deserializing (this=0x7ffff697dfe0, __in_chrg=<optimized out>)

at /home/biancacr/clang_build_/src/tools/clang/include/clang/AST/ExternalASTSource.h:69

#4 0x0000000003b4ae73 in clang::ASTReader::ReadAST (this=0x7ffff001b620, FileName=..., Type=clang::serialization::MK_ImplicitModule, ImportLoc=..., ClientLoadCapabilities=1)

at /home/biancacr/clang_build_/src/tools/clang/lib/Serialization/ASTReader.cpp:3486

#5 0x0000000002c6f891 in compileAndLoadModule (ImportingInstance=..., ImportLoc=..., ModuleNameLoc=..., Module=0x7ffff000de30, ModuleFileName=...)

at /home/biancacr/clang_build_/src/tools/clang/lib/Frontend/CompilerInstance.cpp:1124

#6 0x0000000002c7261d in clang::CompilerInstance::loadModule (this=0x7fffffffa7a0, ImportLoc=..., Path=..., Visibility=clang::Module::Hidden, IsInclusionDirective=true)

and finishPendingActions which calls loadDeclUpdateRecords finishes before the callback to ResolveExceptionSpec happens.

Thus we set it in FinishedDeserializing, is it not causing updates to be missed? Clang testsuite passes though.

Another sugesstion whould have been to make the flag part of the ExternalASTSource which triggers the Deserializing.

Is this the right direction?

Thanks!

[rsmith]

This seems like a good approach, but I'd like to see it generalized:

• add an RAII object to set the "processing updates" flag
• also set the flag when loading decl update records
• also check for the flag in all the other ASTMutationListener callbacks on the ASTWriter

This should let us address the various FIXMEs in the ASTReader where we mutate the AST and fail to produce the relevant calls to the mutation listeners. (Search for "doesn't send the right notifications" in ASTReaderDecl.cpp.)

[CrisCristescu]

This patch addresses the comments from the previous one:

add an RAII object to set the "processing updates" flag - done
also set the flag when loading decl update records - set but not sufficient only in the loadDeclUpdateRecords. 
also check for the flag in all the other ASTMutationListener callbacks on the ASTWriter - done

This should let us address the various FIXMEs in the ASTReader where we mutate the AST and fail to produce the relevant calls to the mutation listeners. (Search for "doesn't send the right notifications" in ASTReaderDecl.cpp.) - done

Clang test passes happily.
There is the common problem of RAII objects of nested RAII objects. The created RAII object does not cater for nested calls or interleaved calls, but this limitation should not affect our particular case. If you believe it will and you think we should implemented a RAII object which supports nested call we can do that too.

[CrisCristescu]

Some cosmetics changes.

[CrisCristescu]

More cosmetics.

[v.g.vassilev]

LGTM.

[rsmith]

This looks great, thanks! A couple of minor things then this is good to go.

include/clang/Serialization/ASTReader.h

1060

Please do something to catch the case where we have multiple of these objects live at once -- either save and restore this flag in the constructor and destructor, or assert that it's false in the constructor.

lib/Serialization/ASTReader.cpp

8634–8637

Sink the object into the loop that performs updates, so it doesn't cover unrelated things that happen below, such as passing decls to the AST consumer.

lib/Serialization/ASTReaderDecl.cpp

3930–3931

Can we remove this flag entirely now?

[CrisCristescu]

Last comments addressed.

[v.g.vassilev]

Landed in r276473.

include/clang/Serialization/ASTReader.h

private:
   bool ValidateSystemInputs;
 
   /// \brief Whether we are allowed to use the global module index.
   bool UseGlobalIndex;
 
   /// \brief Whether we have tried loading the global module index yet.
   bool TriedLoadingGlobalIndex;
 
+  ///\brief Whether we are currently processing update records.
+  bool ProcessingUpdateRecords;
+
   typedef llvm::DenseMap<unsigned, SwitchCase *> SwitchCaseMapTy;
   /// \brief Mapping from switch-case IDs in the chain to switch-case statements
   ///
   /// Statements usually don't have IDs, but switch cases need them, so that the
   /// switch statement can refer to them.
   SwitchCaseMapTy SwitchCaseStmts;
 
   SwitchCaseMapTy *CurrSwitchCaseStmts;
public:
     ReadingKindTracker(enum ReadingKind newKind, ASTReader &reader)
       : Reader(reader), PrevKind(Reader.ReadingKind) {
       Reader.ReadingKind = newKind;
     }
 
     ~ReadingKindTracker() { Reader.ReadingKind = PrevKind; }
   };
 
+  /// \brief RAII object to mark the start of processing updates.
+  class ProcessingUpdatesRAIIObj {
+    ASTReader &Reader;
+    bool PrevState;
+
+    ProcessingUpdatesRAIIObj(const ProcessingUpdatesRAIIObj &) = delete;
+    void operator=(const ProcessingUpdatesRAIIObj &) = delete;
+
+  public:
+    ProcessingUpdatesRAIIObj(ASTReader &reader)
+      : Reader(reader), PrevState(Reader.ProcessingUpdateRecords) {
+      Reader.ProcessingUpdateRecords = true;
+    }
+
+    ~ProcessingUpdatesRAIIObj() { Reader.ProcessingUpdateRecords = PrevState; }
+  };
+
   /// \brief Suggested contents of the predefines buffer, after this
   /// PCH file has been processed.
   ///
   /// In most cases, this string will be empty, because the predefines
   /// buffer computed to build the PCH file will be identical to the
   /// predefines buffer computed from the command line. However, when
   /// there are differences that the PCH reader can work around, this
   /// predefines buffer may contain additional definitions.
public:
   void ClearSwitchCaseIDs();
 
   /// \brief Cursors for comments blocks.
   SmallVector<std::pair<llvm::BitstreamCursor,
                         serialization::ModuleFile *>, 8> CommentsCursors;
 
   /// \brief Loads comments ranges.
   void ReadComments() override;
+
+  bool isProcessingUpdateRecords() { return ProcessingUpdateRecords; }
 };
 
 /// \brief Helper class that saves the current stream position and
 /// then restores it when destroyed.
 struct SavedStreamPosition {
   explicit SavedStreamPosition(llvm::BitstreamCursor &Cursor)
     : Cursor(Cursor), Offset(Cursor.GetCurrentBitNo()) { }
 

lib/Serialization/ASTReader.cpp

assert(NumCurrentElementsDeserializing &&
          "FinishedDeserializing not paired with StartedDeserializing");
   if (NumCurrentElementsDeserializing == 1) {
     // We decrease NumCurrentElementsDeserializing only after pending actions
     // are finished, to avoid recursively re-calling finishPendingActions().
     finishPendingActions();
   }
   --NumCurrentElementsDeserializing;
 
   if (NumCurrentElementsDeserializing == 0) {
     // Propagate exception specification updates along redeclaration chains.
     while (!PendingExceptionSpecUpdates.empty()) {
       auto Updates = std::move(PendingExceptionSpecUpdates);
       PendingExceptionSpecUpdates.clear();
       for (auto Update : Updates) {
+        ProcessingUpdatesRAIIObj ProcessingUpdates(*this);
         auto *FPT = Update.second->getType()->castAs<FunctionProtoType>();
         auto ESI = FPT->getExtProtoInfo().ExceptionSpec;
         if (auto *Listener = Context.getASTMutationListener())
           Listener->ResolvedExceptionSpec(cast<FunctionDecl>(Update.second));
         for (auto *Redecl : Update.second->redecls())
           Context.adjustExceptionSpec(cast<FunctionDecl>(Redecl), ESI);
       }
     }
: Listener(new PCHValidator(PP, *this)), DeserializationListener(nullptr),
       ReadTimer(std::move(ReadTimer)),
       PragmaMSStructState(-1),
       PragmaMSPointersToMembersState(-1),
       isysroot(isysroot), DisableValidation(DisableValidation),
       AllowASTWithCompilerErrors(AllowASTWithCompilerErrors),
       AllowConfigurationMismatch(AllowConfigurationMismatch),
       ValidateSystemInputs(ValidateSystemInputs),
       UseGlobalIndex(UseGlobalIndex), TriedLoadingGlobalIndex(false),
+      ProcessingUpdateRecords(false),
       CurrSwitchCaseStmts(&SwitchCaseStmts), NumSLocEntriesRead(0),
       TotalNumSLocEntries(0), NumStatementsRead(0), TotalNumStatements(0),
       NumMacrosRead(0), TotalNumMacros(0), NumIdentifierLookups(0),
       NumIdentifierLookupHits(0), NumSelectorsRead(0),
       NumMethodPoolEntriesRead(0), NumMethodPoolLookups(0),
       NumMethodPoolHits(0), NumMethodPoolTableLookups(0),
       NumMethodPoolTableHits(0), TotalNumMethodPoolEntries(0),
       NumLexicalDeclContextsRead(0), TotalLexicalDeclContexts(0),

lib/Serialization/ASTReaderDecl.cpp

Decl *ASTReader::ReadDeclRecord(DeclID ID) {
 
   return D;
 }
 
 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);
 
     bool WasInteresting = isConsumerInterestedIn(D, false);
     for (auto &FileAndOffset : UpdateOffsets) {
       ModuleFile *F = FileAndOffset.first;
case UPD_CXX_DEDUCED_RETURN_TYPE: {
         // FIXME: If the return type is already deduced, check that it matches.
         FunctionDecl *FD = cast<FunctionDecl>(Redecl);
         Reader.Context.adjustDeducedFunctionResultType(FD, DeducedResultType);
       }
       break;
     }
 
     case UPD_DECL_MARKED_USED: {
-      // FIXME: This doesn't send the right notifications if there are
-      // ASTMutationListeners other than an ASTWriter.
-
       // Maintain AST consistency: any later redeclarations are used too.
-      D->setIsUsed();
+      D->markUsed(Reader.Context);
       break;
     }
 
     case UPD_MANGLING_NUMBER:
       Reader.Context.setManglingNumber(cast<NamedDecl>(D), Record[Idx++]);
       break;
 
     case UPD_STATIC_LOCAL_NUMBER:
       Reader.Context.setStaticLocalNumber(cast<VarDecl>(D), Record[Idx++]);
       break;
 
     case UPD_DECL_MARKED_OPENMP_THREADPRIVATE:
       D->addAttr(OMPThreadPrivateDeclAttr::CreateImplicit(
           Reader.Context, ReadSourceRange(Record, Idx)));
       break;
 
     case UPD_DECL_EXPORTED: {
       unsigned SubmoduleID = readSubmoduleID(Record, Idx);
       auto *Exported = cast<NamedDecl>(D);
       if (auto *TD = dyn_cast<TagDecl>(Exported))
         Exported = TD->getDefinition();
       Module *Owner = SubmoduleID ? Reader.getSubmodule(SubmoduleID) : nullptr;
       if (Reader.getContext().getLangOpts().ModulesLocalVisibility) {
-        // FIXME: This doesn't send the right notifications if there are
-        // ASTMutationListeners other than an ASTWriter.
-        Reader.getContext().mergeDefinitionIntoModule(
-            cast<NamedDecl>(Exported), Owner,
-            /*NotifyListeners*/ false);
+        Reader.getContext().mergeDefinitionIntoModule(cast<NamedDecl>(Exported),
+                                                      Owner);
         Reader.PendingMergedDefinitionsToDeduplicate.insert(
             cast<NamedDecl>(Exported));
       } else if (Owner && Owner->NameVisibility != Module::AllVisible) {
         // If Owner is made visible at some later point, make this declaration
         // visible too.
         Reader.HiddenNamesMap[Owner].push_back(Exported);
       } else {
         // The declaration is now visible.

lib/Serialization/ASTWriter.cpp

 }
 
 void ASTWriter::ModuleRead(serialization::SubmoduleID ID, Module *Mod) {
   assert(SubmoduleIDs.find(Mod) == SubmoduleIDs.end());
   SubmoduleIDs[Mod] = ID;
 }
 
 void ASTWriter::CompletedTagDefinition(const TagDecl *D) {
+  if (Chain && Chain->isProcessingUpdateRecords()) return;
   assert(D->isCompleteDefinition());
   assert(!WritingAST && "Already writing the AST!");
   if (auto *RD = dyn_cast<CXXRecordDecl>(D)) {
     // We are interested when a PCH decl is modified.
     if (RD->isFromASTFile()) {
       // A forward reference was mutated into a definition. Rewrite it.
       // FIXME: This happens during template instantiation, should we
       // have created a new definition decl instead ?
if (D->isFromASTFile())
     return true;
 
   // The predefined __va_list_tag struct is imported if we imported any decls.
   // FIXME: This is a gross hack.
   return D == D->getASTContext().getVaListTagDecl();
 }
 
 void ASTWriter::AddedVisibleDecl(const DeclContext *DC, const Decl *D) {
-   assert(DC->isLookupContext() &&
+  if (Chain && Chain->isProcessingUpdateRecords()) return;
           "Should not add lookup results to non-lookup contexts!");
 
   // TU is handled elsewhere.
   if (isa<TranslationUnitDecl>(DC))
     return;
 
   // Namespaces are handled elsewhere, except for template instantiations of
   // FunctionTemplateDecls in namespaces. We are interested in cases where the
if (UpdatedDeclContexts.insert(DC) && !cast<Decl>(DC)->isFromASTFile()) {
     // surprise when we try to emit its lookup table.
     for (auto *Child : DC->decls())
       UpdatingVisibleDecls.push_back(Child);
   }
   UpdatingVisibleDecls.push_back(D);
 }
 
 void ASTWriter::AddedCXXImplicitMember(const CXXRecordDecl *RD, const Decl *D) {
+  if (Chain && Chain->isProcessingUpdateRecords()) return;
   assert(D->isImplicit());
 
   // We're only interested in cases where a local declaration is added to an
   // imported context.
   if (D->isFromASTFile() || !isImportedDeclContext(Chain, RD))
     return;
 
   if (!isa<CXXMethodDecl>(D))
     return;
 
   // A decl coming from PCH was modified.
   assert(RD->isCompleteDefinition());
   assert(!WritingAST && "Already writing the AST!");
   DeclUpdates[RD].push_back(DeclUpdate(UPD_CXX_ADDED_IMPLICIT_MEMBER, D));
 }
 
 void ASTWriter::ResolvedExceptionSpec(const FunctionDecl *FD) {
+  if (Chain && Chain->isProcessingUpdateRecords()) return;
   assert(!DoneWritingDeclsAndTypes && "Already done writing updates!");
   if (!Chain) return;
   Chain->forEachImportedKeyDecl(FD, [&](const Decl *D) {
     // If we don't already know the exception specification for this redecl
     // chain, add an update record for it.
     if (isUnresolvedExceptionSpec(cast<FunctionDecl>(D)
                                       ->getType()
                                       ->castAs<FunctionProtoType>()
                                       ->getExceptionSpecType()))
       DeclUpdates[D].push_back(UPD_CXX_RESOLVED_EXCEPTION_SPEC);
   });
 }
 
 void ASTWriter::DeducedReturnType(const FunctionDecl *FD, QualType ReturnType) {
+  if (Chain && Chain->isProcessingUpdateRecords()) return;
   assert(!WritingAST && "Already writing the AST!");
   if (!Chain) return;
   Chain->forEachImportedKeyDecl(FD, [&](const Decl *D) {
     DeclUpdates[D].push_back(
         DeclUpdate(UPD_CXX_DEDUCED_RETURN_TYPE, ReturnType));
   });
 }
 
 void ASTWriter::ResolvedOperatorDelete(const CXXDestructorDecl *DD,
                                        const FunctionDecl *Delete) {
+  if (Chain && Chain->isProcessingUpdateRecords()) return;
   assert(!WritingAST && "Already writing the AST!");
   assert(Delete && "Not given an operator delete");
   if (!Chain) return;
   Chain->forEachImportedKeyDecl(DD, [&](const Decl *D) {
     DeclUpdates[D].push_back(DeclUpdate(UPD_CXX_RESOLVED_DTOR_DELETE, Delete));
   });
 }
 
 void ASTWriter::CompletedImplicitDefinition(const FunctionDecl *D) {
+  if (Chain && Chain->isProcessingUpdateRecords()) return;
   assert(!WritingAST && "Already writing the AST!");
   if (!D->isFromASTFile())
     return; // Declaration not imported from PCH.
 
   // Implicit function decl from a PCH was defined.
   DeclUpdates[D].push_back(DeclUpdate(UPD_CXX_ADDED_FUNCTION_DEFINITION));
 }
 
 void ASTWriter::FunctionDefinitionInstantiated(const FunctionDecl *D) {
+  if (Chain && Chain->isProcessingUpdateRecords()) return;
   assert(!WritingAST && "Already writing the AST!");
   if (!D->isFromASTFile())
     return;
 
   DeclUpdates[D].push_back(DeclUpdate(UPD_CXX_ADDED_FUNCTION_DEFINITION));
 }
 
 void ASTWriter::StaticDataMemberInstantiated(const VarDecl *D) {
+  if (Chain && Chain->isProcessingUpdateRecords()) return;
   assert(!WritingAST && "Already writing the AST!");
   if (!D->isFromASTFile())
     return;
 
   // Since the actual instantiation is delayed, this really means that we need
   // to update the instantiation location.
   DeclUpdates[D].push_back(
       DeclUpdate(UPD_CXX_INSTANTIATED_STATIC_DATA_MEMBER,
        D->getMemberSpecializationInfo()->getPointOfInstantiation()));
 }
 
 void ASTWriter::DefaultArgumentInstantiated(const ParmVarDecl *D) {
+  if (Chain && Chain->isProcessingUpdateRecords()) return;
   assert(!WritingAST && "Already writing the AST!");
   if (!D->isFromASTFile())
     return;
 
   DeclUpdates[D].push_back(
       DeclUpdate(UPD_CXX_INSTANTIATED_DEFAULT_ARGUMENT, D));
 }
 
 void ASTWriter::AddedObjCCategoryToInterface(const ObjCCategoryDecl *CatD,
                                              const ObjCInterfaceDecl *IFD) {
+  if (Chain && Chain->isProcessingUpdateRecords()) return;
   assert(!WritingAST && "Already writing the AST!");
   if (!IFD->isFromASTFile())
     return; // Declaration not imported from PCH.
   
   assert(IFD->getDefinition() && "Category on a class without a definition?");
   ObjCClassesWithCategories.insert(
     const_cast<ObjCInterfaceDecl *>(IFD->getDefinition()));
 }
 
 void ASTWriter::DeclarationMarkedUsed(const Decl *D) {
+  if (Chain && Chain->isProcessingUpdateRecords()) return;
   assert(!WritingAST && "Already writing the AST!");
 
   // If there is *any* declaration of the entity that's not from an AST file,
   // we can skip writing the update record. We make sure that isUsed() triggers
   // completion of the redeclaration chain of the entity.
   for (auto Prev = D->getMostRecentDecl(); Prev; Prev = Prev->getPreviousDecl())
     if (IsLocalDecl(Prev))
       return;
 
   DeclUpdates[D].push_back(DeclUpdate(UPD_DECL_MARKED_USED));
 }
 
 void ASTWriter::DeclarationMarkedOpenMPThreadPrivate(const Decl *D) {
+  if (Chain && Chain->isProcessingUpdateRecords()) return;
   assert(!WritingAST && "Already writing the AST!");
   if (!D->isFromASTFile())
     return;
 
   DeclUpdates[D].push_back(DeclUpdate(UPD_DECL_MARKED_OPENMP_THREADPRIVATE));
 }
 
 void ASTWriter::DeclarationMarkedOpenMPDeclareTarget(const Decl *D,
                                                      const Attr *Attr) {
+  if (Chain && Chain->isProcessingUpdateRecords()) return;
   assert(!WritingAST && "Already writing the AST!");
   if (!D->isFromASTFile())
     return;
 
   DeclUpdates[D].push_back(
       DeclUpdate(UPD_DECL_MARKED_OPENMP_DECLARETARGET, Attr));
 }
 
 void ASTWriter::RedefinedHiddenDefinition(const NamedDecl *D, Module *M) {
+  if (Chain && Chain->isProcessingUpdateRecords()) return;
   assert(!WritingAST && "Already writing the AST!");
   assert(D->isHidden() && "expected a hidden declaration");
   DeclUpdates[D].push_back(DeclUpdate(UPD_DECL_EXPORTED, M));
 }
 
 void ASTWriter::AddedAttributeToRecord(const Attr *Attr,
                                        const RecordDecl *Record) {
+  if (Chain && Chain->isProcessingUpdateRecords()) return;
   assert(!WritingAST && "Already writing the AST!");
   if (!Record->isFromASTFile())
     return;
   DeclUpdates[Record].push_back(DeclUpdate(UPD_ADDED_ATTR_TO_RECORD, Attr));
 }

test/Modules/Inputs/PR28332/TextualInclude.h

+#ifndef LLVM_ADT_SMALLVECTORIMPL_H
+#define LLVM_ADT_SMALLVECTORIMPL_H
+class SmallVectorImpl {
+public:
+  ~SmallVectorImpl();
+};
+#endif
 No newline at end of file

test/Modules/Inputs/PR28332/a.h

+#include "b.h"
+
+class A {
+  SmallVector<char, 8> LegalIntWidths;
+  A() {}
+};
+
+#include "c.h"

test/Modules/Inputs/PR28332/b.h

+#include "TextualInclude.h"
+template <typename, int> class SmallVector : SmallVectorImpl {};
+

test/Modules/Inputs/PR28332/c.h

+#include "TextualInclude.h"
+

test/Modules/Inputs/PR28332/module.modulemap

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

test/Modules/pr28332.cpp

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++11 -I%S/Inputs/PR28332 -verify %s
+// RUN: %clang_cc1 -std=c++11 -fmodules -fmodule-map-file=%S/Inputs/PR28332/module.modulemap -fmodules-cache-path=%t -I%S/Inputs/PR28332 -verify %s
+
+#include "a.h"
+
+// expected-no-diagnostics
+