[PATCH] D15463: [Objective-c] Fix a crash
Justin Bogner via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 15 16:55:51 PST 2015
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.
>> >> >
>> >> >
>> >>
>>
More information about the cfe-commits
mailing list