Page MenuHomePhabricator

[ObjC] Prevent infinite loops when iterating over redeclaration of a method that was declared in an invalid interface
ClosedPublic

Authored by arphaman on Nov 15 2016, 4:53 AM.

Details

Summary

This patch fixes an infinite loop that occurs when clang tries to iterate over redeclaration of a method that was declared in an invalid @interface. The existing validity checks don't catch this as that @interface is a duplicate of a previously declared valid @interface declaration, so we have to verify that the found redeclaration is in a valid declaration context.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman updated this revision to Diff 77980.Nov 15 2016, 4:53 AM
arphaman retitled this revision from to [ObjC] Prevent infinite loops when iterating over redeclaration of a method that was declared in an invalid interface.
arphaman updated this object.
arphaman added reviewers: manmanren, mehdi_amini.
arphaman set the repository for this revision to rL LLVM.
arphaman added a subscriber: cfe-commits.
mehdi_amini accepted this revision.Nov 15 2016, 9:06 AM
mehdi_amini edited edge metadata.

LGTM, thks.
(See one inline comment to fix though)

lib/AST/DeclObjC.cpp
841 ↗(On Diff #77980)

Either you should use dyn_cast, or you should just cast but without a if

This revision is now accepted and ready to land.Nov 15 2016, 9:06 AM
arphaman added inline comments.Nov 15 2016, 9:15 AM
lib/AST/DeclObjC.cpp
841 ↗(On Diff #77980)

Thanks! Yeah, this is supposed to be dyn_cast.

erik.pilkington added inline comments.
lib/AST/DeclObjC.cpp
841 ↗(On Diff #77980)

Why dyn_cast? Can't we bank on a DeclContext being a Decl?

888 ↗(On Diff #77980)

Do you think it would be cleaner to just do the check once here, if Redecl is non-null?

arphaman added inline comments.Nov 15 2016, 10:06 AM
lib/AST/DeclObjC.cpp
841 ↗(On Diff #77980)

I think you're right, I just need a cast without an if since DeclContext subclasses are all Decls.

888 ↗(On Diff #77980)

Yes, that would be neater. Thanks for the suggestion!

This revision was automatically updated to reflect the committed changes.
arphaman marked 6 inline comments as done.

I managed to get rid of the extra function and simplify the check thanks to suggestions from Erik.