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

[tahonermann]

Summary

This patch corrects several errors that result in incorrect Objective-C method declaration enumeration that occurs when an AST is deserialized from an .ast file.

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.

[tahonermann]

The formatting I intended in the summary did not take affect. Here is the referenced code as I intended it to display:

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.

[tahonermann]

lib/AST/DeclObjC.cpp

1745

The subtle fix. Calling ImplD->getIdentifier() is required to get the category name. this->getIdentifier() returns the class name.

[klimek]

+richard

[rsmith]

Please add a testcase (I'd imagine it's not too hard to craft one in this case?).

lib/AST/DeclObjC.cpp

1745

Ouch. Can I tempt you to work on fixing the hiding of getIdentifier after this patch lands? :)

lib/Serialization/ASTReaderDecl.cpp

984–985

Don't add PR references to the code; your comments should be complete enough to explain why we're doing what we're doing now without reference to what we got wrong before. In this case, I think they are. (People interested in the code's history can check the SVN logs.)

[tahonermann]

Please add a testcase (I'd imagine it's not too hard to craft one in this case?).

I had posted the test case separately. You can find it in D9126. I had to craft a unit test. I couldn't find a way to exploit the problem from just the Clang command line.

lib/AST/DeclObjC.cpp

1745

Oh, I was already tempted. I'll try and find some time to fix it. I'll be surprised if there aren't other cases like this waiting to be found.

lib/Serialization/ASTReaderDecl.cpp

984–985

Funny. Usually I'm the one telling people not to add such references. Would you like me to repost the patch with the PR reference removed?

[rsmith]

OK, the combination of these two changes LGTM (modulo renaming the test file and removing the PR number). For future reference, it's our convention to include the code change and tests in the same review.

Do you need someone to commit this for you?

lib/Serialization/ASTReaderDecl.cpp

984–985

Nah, whoever commits can just do that before checkin.

[tahonermann]

For future reference, it's our convention to include the code change and tests in the same review.

Thanks, I'll do so in the future.

Do you need someone to commit this for you?

Yes, please. I don't have commit access.

[klimek]

Richard, was there a reason you didn't submit this?

[Eugene.Zelenko]

Looks like patch was not committed.

[tahonermann]

For what it's worth, Coverity has been running with this patch in place for at least a year and a half now. I believe the only reason it wasn't committed was due to concerns with the test case in D9126. I haven't had time to address those concerns (and don't expect to anytime soon).

lib/AST/DeclObjC.cpp

void ObjCImplDecl::setClassInterface(ObjCInterfaceDecl *IFace) {
 
   if (ObjCImplementationDecl *ImplD
         = dyn_cast_or_null<ObjCImplementationDecl>(this)) {
     if (IFace)
       Ctx.setObjCImplementation(IFace, ImplD);
 
   } else if (ObjCCategoryImplDecl *ImplD =
              dyn_cast_or_null<ObjCCategoryImplDecl>(this)) {
-    if (ObjCCategoryDecl *CD = IFace->FindCategoryDeclaration(getIdentifier()))
+    if (ObjCCategoryDecl *CD =
+        IFace->FindCategoryDeclaration(ImplD->getIdentifier())) {
       Ctx.setObjCImplementation(CD, ImplD);
     }
+  }
 
   ClassInterface = IFace;
 }
 
 /// FindPropertyImplIvarDecl - This method lookup the ivar in the list of
 /// properties implemented in this \@implementation block and returns
 /// the implemented property that uses it.
 ///

lib/Serialization/ASTReaderDecl.cpp

void ASTDeclReader::VisitObjCPropertyDecl(ObjCPropertyDecl *D) {
   D->setSetterName(Reader.ReadDeclarationName(F,Record, Idx).getObjCSelector());
   D->setGetterMethodDecl(ReadDeclAs<ObjCMethodDecl>(Record, Idx));
   D->setSetterMethodDecl(ReadDeclAs<ObjCMethodDecl>(Record, Idx));
   D->setPropertyIvarDecl(ReadDeclAs<ObjCIvarDecl>(Record, Idx));
 }
 
 void ASTDeclReader::VisitObjCImplDecl(ObjCImplDecl *D) {
   VisitObjCContainerDecl(D);
-  D->setClassInterface(ReadDeclAs<ObjCInterfaceDecl>(Record, Idx));
+  // For ObjCCategoryImplDecl, setClassInterface must not be called until the
+  // category identifier has been deserialized and set.  See PR23175.
 }
 
 void ASTDeclReader::VisitObjCCategoryImplDecl(ObjCCategoryImplDecl *D) {
   VisitObjCImplDecl(D);
   D->setIdentifier(Reader.GetIdentifierInfo(F, Record, Idx));
   D->CategoryNameLoc = ReadSourceLocation(Record, Idx);
+  D->setClassInterface(ReadDeclAs<ObjCInterfaceDecl>(Record, Idx));
 }
 
 void ASTDeclReader::VisitObjCImplementationDecl(ObjCImplementationDecl *D) {
   VisitObjCImplDecl(D);
+  D->setClassInterface(ReadDeclAs<ObjCInterfaceDecl>(Record, Idx));
   D->setSuperClass(ReadDeclAs<ObjCInterfaceDecl>(Record, Idx));
   D->SuperLoc = ReadSourceLocation(Record, Idx);
   D->setIvarLBraceLoc(ReadSourceLocation(Record, Idx));
   D->setIvarRBraceLoc(ReadSourceLocation(Record, Idx));
   D->setHasNonZeroConstructors(Record[Idx++]);
   D->setHasDestructors(Record[Idx++]);
   D->NumIvarInitializers = Record[Idx++];
   if (D->NumIvarInitializers)

lib/Serialization/ASTWriterDecl.cpp

void ASTDeclWriter::VisitObjCPropertyDecl(ObjCPropertyDecl *D) {
   Writer.AddDeclRef(D->getGetterMethodDecl(), Record);
   Writer.AddDeclRef(D->getSetterMethodDecl(), Record);
   Writer.AddDeclRef(D->getPropertyIvarDecl(), Record);
   Code = serialization::DECL_OBJC_PROPERTY;
 }
 
 void ASTDeclWriter::VisitObjCImplDecl(ObjCImplDecl *D) {
   VisitObjCContainerDecl(D);
-  Writer.AddDeclRef(D->getClassInterface(), Record);
   // Abstract class (no need to define a stable serialization::DECL code).
 }
 
 void ASTDeclWriter::VisitObjCCategoryImplDecl(ObjCCategoryImplDecl *D) {
   VisitObjCImplDecl(D);
   Writer.AddIdentifierRef(D->getIdentifier(), Record);
   Writer.AddSourceLocation(D->getCategoryNameLoc(), Record);
+  Writer.AddDeclRef(D->getClassInterface(), Record);
   Code = serialization::DECL_OBJC_CATEGORY_IMPL;
 }
 
 void ASTDeclWriter::VisitObjCImplementationDecl(ObjCImplementationDecl *D) {
   VisitObjCImplDecl(D);
+  Writer.AddDeclRef(D->getClassInterface(), Record);
   Writer.AddDeclRef(D->getSuperClass(), Record);
   Writer.AddSourceLocation(D->getSuperClassLoc(), Record);
   Writer.AddSourceLocation(D->getIvarLBraceLoc(), Record);
   Writer.AddSourceLocation(D->getIvarRBraceLoc(), Record);
   Record.push_back(D->hasNonZeroConstructors());
   Record.push_back(D->hasDestructors());
   Record.push_back(D->NumIvarInitializers);
   if (D->NumIvarInitializers)