[PATCH] D15463: [Objective-c] Fix a crash

[Akira Hatanaka via cfe-commits]

On Mon, Dec 14, 2015 at 10:39 AM, Justin Bogner <mail at justinbogner.com>
wrote:

> Akira Hatanaka via cfe-commits <cfe-commits at lists.llvm.org> writes:
> > ahatanak created this revision.
> > ahatanak added a subscriber: cfe-commits.
> >
> > This patch fixes a crash that occurs when __kindof is incorrectly used
> > in the type parameter list of an interface. The crash occurs because
> > ObjCTypeParamList::back() is called in checkTypeParamListConsistency
> > on an empty list:
> >
> > 00762       diagLoc =
> S.getLocForEndOfToken(newTypeParams->back()->getLocEnd());
> >
> > http://reviews.llvm.org/D15463
> >
> > Files:
> >   lib/Parse/ParseObjc.cpp
> >   test/SemaObjC/kindof.m
> >
> > Index: test/SemaObjC/kindof.m
> > ===================================================================
> > --- test/SemaObjC/kindof.m
> > +++ test/SemaObjC/kindof.m
> > @@ -302,3 +302,13 @@
> >    void processCopyable(__typeof(getSomeCopyable()) string);
> >    processCopyable(0); // expected-warning{{null passed to a callee that
> requires a non-null argument}}
> >  }
> > +
> > +// __kinddof cannot be used in parameter list.
> > + at interface Array1<T> : NSObject
> > + at end
> > +
> > + at interface I1 : NSObject
> > + at end
> > +
> > + at interface Array1<__kindof I1*>(extensions) // //
> expected-error{{expected type parameter name}}
> > + at end
> > Index: lib/Parse/ParseObjc.cpp
> > ===================================================================
> > --- lib/Parse/ParseObjc.cpp
> > +++ lib/Parse/ParseObjc.cpp
> > @@ -603,7 +603,7 @@
> >    // whether there are any protocol references.
> >    lAngleLoc = SourceLocation();
> >    rAngleLoc = SourceLocation();
> > -  return list;
> > +  return invalid ? nullptr : list;
>
> This looks a bit suspicious to me. Since `invalid` is set *way* earlier
> in the function, it seems like we should be able to return earlier if
> this is correct, and not even call actOnObjCTypeParamList. OTOH, are
> there cases where `invalid == true` but list is non-empty? If so, are we
> doing the right thing when that happens?
>
>
I'm assuming you meant it should return earlier if this is *incorrect* (but
let me know if I misunderstood your comment).

The only downside to returning nullptr before actOnObjCTypeParamList is
called in line 595 is that it won't print the diagnostics that are printed
in actOnObjCTypeParamList. For example, if the following interface were
compiled,

@interface I1<T, T, __kindof I2*> NSObject
@end

actOnObjCTypeParamList could tell the user that parameter "T" was
redeclared in addition to printing the error message about __kindof. So if
we return before it's called, we won't see that error message.

When the interface declaration of category extensions1 in the following
piece of code is compiled, invalid is set to true and the list is not empty
(it contains the first two parameters).

@interface I1<T1, T2, T3> NSObject
@end

@interface I1<T, T, __kindof I2*> (extensions1)
@end

*test3.m:6:23: **error: **expected type parameter name*

@interface I6<T1, T2, __kindof I2*> (extensions1)

*                      ^*

*test3.m:6:21: **error: **category has too few type parameters (expected 3,
have 2)*

@interface I6<T1, T2, __kindof I2*> (extensions1)
It looks like it's doing the right thing, but the second diagnostic doesn't
seem necessary to me.

>  }
> >
> >  /// Parse an objc-type-parameter-list.
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151214/f0a23f2c/attachment.html>