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 ↗

(On Diff #64669)

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 ↗

(On Diff #64669)

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

3932 ↗

(On Diff #64669)

Can we remove this flag entirely now?

[CrisCristescu]

Last comments addressed.

[v.g.vassilev]

Landed in r276473.

lib/Serialization/ASTWriter.cpp

void ASTWriter::AddedCXXImplicitMember(const CXXRecordDecl *RD, const Decl *D) {
 
   // 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 (FD->isFromASTFile())
+    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>()