Page MenuHomePhabricator

Fix for Bug 28332
ClosedPublic

Authored by CrisCristescu on Jun 28 2016, 9:27 AM.

Details

Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

CrisCristescu retitled this revision from to Fix for Bug 28332.
CrisCristescu updated this object.
CrisCristescu added reviewers: rsmith, v.g.vassilev.
CrisCristescu set the repository for this revision to rL LLVM.
CrisCristescu added a project: Restricted Project.
rsmith edited edge metadata.Jun 28 2016, 3:41 PM

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.

rsmith requested changes to this revision.Jun 28 2016, 3:41 PM
rsmith edited edge metadata.
This revision now requires changes to proceed.Jun 28 2016, 3:41 PM

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 edited edge metadata.
CrisCristescu removed a project: Restricted Project.

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!

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 edited edge metadata.

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 added a reviewer: aprantl.

Some cosmetics changes.

More cosmetics.

v.g.vassilev accepted this revision.Jul 20 2016, 6:38 AM
v.g.vassilev edited edge metadata.

LGTM.

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

include/clang/Serialization/ASTReader.h
1061

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
3938

Can we remove this flag entirely now?

CrisCristescu edited edge metadata.

Last comments addressed.

rsmith accepted this revision.Jul 22 2016, 1:22 PM
rsmith edited edge metadata.
This revision is now accepted and ready to land.Jul 22 2016, 1:22 PM
v.g.vassilev closed this revision.Jul 22 2016, 2:16 PM

Landed in r276473.