Infinite loop iterating Objective-C method declarations in categories when the AST was deserialized from an .ast file

[Quuxplusone]

Bugzilla Link	PR23175
Status	NEW
Importance	P normal
Reported by	Tom Honermann (thonerma@synopsys.com)
Reported on	2015-04-09 13:50:51 -0700
Last modified on	2015-04-13 08:58:52 -0700
Version	trunk
Hardware	PC Linux
CC	llvm-bugs@lists.llvm.org, sig-rnd-sat-clang-bugs@synopsys.com
Fixed by commit(s)
Attachments	pr23175-unit-test.patch (9370 bytes, text/plain) pr23175-fix.patch (2843 bytes, text/plain)
Blocks
Blocked by
See also

I encountered an infinite loop in Clang 3.4 - trunk (r234313) when iterating over ObjCMethodDecl declarations using the Decl::redecl_begin() and Decl::redecl_end() interfaces for methods declared in both category and category implementation declarations when the AST has been deserialized from a .ast file. Iteration terminates correctly when the AST has not been serialized and then deserialized.

I haven't been able to reproduce this issue using the clang command line. I have a unit test that I will attach.

[Quuxplusone]

Attached pr23175-unit-test.patch (9370 bytes, text/plain): Patch providing a unit test exhibiting the issue

[Quuxplusone]

A few notes about the attached unit test:

1) The path was produced based on r234313.
2) This adds a new unittests/Serialization directory and unit test to exercise
   AST matching on an AST that has been serialized and deserialized.
3) I'm not proficient with the AST matchers.  There may be a better way to
   handle this.  I added a declarationCountIs() AST matcher at one point, but
   I lacked convenient infrastructure like that available in unittests/ASTMatchers
   to validate the matches.
4) This adds an additional parameter with default argument to buildASTFromCode()
   and buildASTFromCodeWithArgs() that is used to indicate whether the code
   should be serialized and deserialized prior to returning the AST.

The test currently fails, but without going into an infinite loop (iteration is
terminated if more than the expected number of declarations are enumerated).
Passing 'true' for 'Reserialize' to buildASTFromCode() in the new test suffices
to make the test pass, thus demonstrating that the issue is related to AST
serialization and deserialization.

[Quuxplusone]

Attached pr23175-fix.patch (2843 bytes, text/plain): Patch with the proposed fix

[Quuxplusone]

I attached a patch with a proposed fix.  There were two issues:

1) In lib/AST/DeclObjC.cpp, in ObjCImplDecl::setClassInterface(), the base class
   version (ObjCImplDecl) of (non-virtual) getIdentifier() was being called, but
   the derived class version (ObjCCategoryImplDecl) is what is required.  See
   this comment in include/clang/AST/DeclObjC.h:
     class ObjCCategoryImplDecl : public ObjCImplDecl {
     ...
     /// getIdentifier - Get the identifier that names the category
     /// interface associated with this implementation.
     /// FIXME: This is a bad API, we are hiding NamedDecl::getIdentifier()
     /// with a different meaning. For example:
     /// ((NamedDecl *)SomeCategoryImplDecl)->getIdentifier()
     /// returns the class interface name, whereas
     /// ((ObjCCategoryImplDecl *)SomeCategoryImplDecl)->getIdentifier()
     /// returns the category name.
     IdentifierInfo *getIdentifier() const {
       return Id;
     }
   I did not change the interface, but clearly, it would be a good idea to do
   so.  The patch uses the existing downcasted pointer to call the correct
   version.

2) With the above change, when deserializing a ObjCCategoryImplDecl instance,
   ObjCImplDecl::setClassInterface() was being called by
   ASTDeclReader::VisitObjCImplDecl() before the category name was deserialized
   in ASTDeclReader::VisitObjCCategoryImplDecl().  The patch delays the call to
   ObjCImplDecl::setClassInterface() until after the category name is
   deserialized and set.

[Quuxplusone]

The unit test in the patch attached as attachment 14175 contains a couple of
lines I had added for test purposes and did not intend to leave in the patch.
They are:

unittests/Serialization/Reserialization.cp:
+    "void f();\n"
+    "void f();\n"

Please remove these lines if/when the patch is committed.