[PATCH] D15463: [Objective-c] Fix a crash
Akira Hatanaka via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 15 22:31:07 PST 2015
Thank you for the review. I added a test to check the "redeclaration of
type parameter" error message and committed it as r255754.
On Tue, Dec 15, 2015 at 4:55 PM, Justin Bogner <mail at justinbogner.com>
wrote:
> Akira Hatanaka <ahatanak at gmail.com> writes:
> > On Tue, Dec 15, 2015 at 12:55 PM, Justin Bogner <mail at justinbogner.com>
> > wrote:
> >
> >> Akira Hatanaka <ahatanak at gmail.com> writes:
> >> > 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).
> >>
> >> I meant, "If `return invalid ? nullptr : list` is correct, then I
> >> suspect returning nullptr before calling actOnObjCTypeParamList makes
> >> more sense.
> >>
> >> > 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.
> >>
> >> I'm not sure I follow exactly which errors are being reported with your
> >> patch, so maybe this is already working, but what we basically want is
> >> to only emit errors that are helpful.
> >>
> >> That is, if we get the `"T" is a redeclaration` error as well as the
> >> __kindof error, that's nice, but if we get the `too few type parameters`
> >> error that's going to be confusing and bad - the user didn't provide too
> >> few type parameters, but our previous error caused us to see too few.
> >>
> >> Basically, we don't ever want to report errors that are the result of
> >> previous errors, but as long as we don't end up doing that then
> >> gathering further errors is totally reasonable. Make sense?
> >>
> >>
> > Yes, that makes perfect sense.
>
> Thanks for the detailed explanation! This LGTM then.
>
> > The following shows the error messages I see when I compile test case
> > test3.m:
> >
> > $ cat test3.m
> >
> > #import <Foundation/Foundation.h>
> > @interface I2 : NSObject
> > @end
> >
> > @interface I7<T0, T1, T2> : NSObject
> > @end
> >
> > @interface I7<T, T, __kindof I2*>(ext2)
> > @end
> >
> > @interface I6<T> : NSObject
> > @end
> >
> > @interface I6<__kindof I2*>(ext1)
> > @end
> >
> >
> > ### This patch: it prints the error messages we want to see.
> > test3.m:9:21: error: expected type parameter name
> > @interface I7<T, T, __kindof I2*>(ext2)
> > ^
> >
> > test3.m:9:18: error: redeclaration of type parameter 'T'
> > @interface I7<T, T, __kindof I2*>(ext2)
> > ~ ^
> >
> > test3.m:15:15: error: expected type parameter name
> > @interface I6<__kindof I2*>(ext1)
> > ^
> >
> > 3 errors generated.
> >
> > ### If it returns nullptr before actOnObjCTypeParamList is called: it
> > doesn't print the "redeclaration of type parameter" error.
> >
> > test3.m:9:21: error: expected type parameter name
> > @interface I7<T, T, __kindof I2*>(ext2)
> > ^
> >
> > test3.m:15:15: error: expected type parameter name
> > @interface I6<__kindof I2*>(ext1)
> > ^
> >
> > 2 errors generated.
> >
> > ### Trunk: It prints the "too few type parameters" message and it
> crashes.
> >
> > test3.m:9:21: error: expected type parameter name
> > @interface I7<T, T, __kindof I2*>(ext2)
> > ^
> >
> > test3.m:9:18: error: redeclaration of type parameter 'T'
> > @interface I7<T, T, __kindof I2*>(ext2)
> > ~ ^
> >
> > test3.m:9:19: error: category has too few type parameters (expected 3,
> have
> > 2)
> > @interface I7<T, T, __kindof I2*>(ext2)
> > ^
> >
> > test3.m:15:15: error: expected type parameter name
> > @interface I6<__kindof I2*>(ext1)
> > ^
> >
> > clang: error: unable to execute command: Segmentation fault: 11
> >
> >>
> >> >> }
> >> >> >
> >> >> > /// Parse an objc-type-parameter-list.
> >> >> >
> >> >> >
> >> >>
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151215/abf6756a/attachment.html>
More information about the cfe-commits
mailing list