| Message ID | 549AF7D6.5000708@gmail.com |
|---|---|
| State | New |
| Headers | show |
Ping! On 12/24/2014 07:28 PM, Dimitris Papavasiliou wrote: > Hello, > > The attached patch fixes an issue reported a couple of years ago in Bug > 51891 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51891). The problem > is caused because classes without instance variables have no ivar list > at all, so that their ivars pointer is NULL, but the code in > class_copyIvarList () is unaware of this. > > That this is in fact so can be easily verified by checking the code of > class_addIvar in the same source file, where the ivars list is allocated > when the first ivar is added. The code there also checks for a NULL > ivars pointer. > > The patch also adds a simple test-case for this issue. I think that the > ChangeLog entry should be something along the lines of: > > 2014-12-24 Dimitris Papavasiliou <dpapavas@gmail.com> > > PR libobjc/51891 > * libobjc/ivars.c: Add a check for classes without instance > variables, which have a NULL ivar list pointer. > * gcc/testsuite/objc.dg/gnu-api-2-class.m: Add a test case > for the above change. > > I hope I got the formatting right. I've run make -k check-objc and all > tests pass without problems. > > Let me know if there are any problems, or if I can do anything else to > facilitate the acceptance of the patch. > > Regards, > Dimitris >
On Dec 24, 2014, at 9:28 AM, Dimitris Papavasiliou <dpapavas@gmail.com> wrote: > The attached patch fixes an issue reported a couple of years ago in Bug 51891 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51891). The problem is caused because classes without instance variables have no ivar list at all, so that their ivars pointer is NULL, but the code in class_copyIvarList () is unaware of this. > > That this is in fact so can be easily verified by checking the code of class_addIvar in the same source file, where the ivars list is allocated when the first ivar is added. The code there also checks for a NULL ivars pointer. > > The patch also adds a simple test-case for this issue. I think that the ChangeLog entry should be something along the lines of: > > 2014-12-24 Dimitris Papavasiliou <dpapavas@gmail.com> > > PR libobjc/51891 > * libobjc/ivars.c: Add a check for classes without instance > variables, which have a NULL ivar list pointer. > * gcc/testsuite/objc.dg/gnu-api-2-class.m: Add a test case > for the above change. > > I hope I got the formatting right. I've run make -k check-objc and all tests pass without problems. > > Let me know if there are any problems, or if I can do anything else to facilitate the acceptance of the patch. > > Regards, > Dimitris So, Andrew is the reviewer for libobjc. I’m not. I don’t have any issue with it. Andrew?
On 01/05/15 14:18, Mike Stump wrote: > On Dec 24, 2014, at 9:28 AM, Dimitris Papavasiliou <dpapavas@gmail.com> wrote: >> The attached patch fixes an issue reported a couple of years ago in Bug 51891 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51891). The problem is caused because classes without instance variables have no ivar list at all, so that their ivars pointer is NULL, but the code in class_copyIvarList () is unaware of this. >> >> That this is in fact so can be easily verified by checking the code of class_addIvar in the same source file, where the ivars list is allocated when the first ivar is added. The code there also checks for a NULL ivars pointer. >> >> The patch also adds a simple test-case for this issue. I think that the ChangeLog entry should be something along the lines of: >> >> 2014-12-24 Dimitris Papavasiliou <dpapavas@gmail.com> >> >> PR libobjc/51891 >> * libobjc/ivars.c: Add a check for classes without instance >> variables, which have a NULL ivar list pointer. >> * gcc/testsuite/objc.dg/gnu-api-2-class.m: Add a test case >> for the above change. >> >> I hope I got the formatting right. I've run make -k check-objc and all tests pass without problems. >> >> Let me know if there are any problems, or if I can do anything else to facilitate the acceptance of the patch. >> >> Regards, >> Dimitris > > So, Andrew is the reviewer for libobjc. I’m not. I don’t have any issue with it. Do you want to be a reviewer for libobjc? I don't think the load there is high, but having someone else who cares about the code is always a good thing. jeff
On Wed, Dec 24, 2014 at 9:28 AM, Dimitris Papavasiliou <dpapavas@gmail.com> wrote: > Hello, > > The attached patch fixes an issue reported a couple of years ago in Bug > 51891 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51891). The problem is > caused because classes without instance variables have no ivar list at all, > so that their ivars pointer is NULL, but the code in class_copyIvarList () > is unaware of this. > > That this is in fact so can be easily verified by checking the code of > class_addIvar in the same source file, where the ivars list is allocated > when the first ivar is added. The code there also checks for a NULL ivars > pointer. > > The patch also adds a simple test-case for this issue. I think that the > ChangeLog entry should be something along the lines of: > > 2014-12-24 Dimitris Papavasiliou <dpapavas@gmail.com> > > PR libobjc/51891 > * libobjc/ivars.c: Add a check for classes without instance > variables, which have a NULL ivar list pointer. > * gcc/testsuite/objc.dg/gnu-api-2-class.m: Add a test case > for the above change. > > I hope I got the formatting right. I've run make -k check-objc and all > tests pass without problems. > > Let me know if there are any problems, or if I can do anything else to > facilitate the acceptance of the patch. This is ok. Thanks, Andrew > > Regards, > Dimitris >
On Thu, Jan 8, 2015 at 9:35 PM, Jeff Law <law@redhat.com> wrote: > On 01/05/15 14:18, Mike Stump wrote: >> >> On Dec 24, 2014, at 9:28 AM, Dimitris Papavasiliou <dpapavas@gmail.com> >> wrote: >>> >>> The attached patch fixes an issue reported a couple of years ago in Bug >>> 51891 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51891). The problem is >>> caused because classes without instance variables have no ivar list at all, >>> so that their ivars pointer is NULL, but the code in class_copyIvarList () >>> is unaware of this. >>> >>> That this is in fact so can be easily verified by checking the code of >>> class_addIvar in the same source file, where the ivars list is allocated >>> when the first ivar is added. The code there also checks for a NULL ivars >>> pointer. >>> >>> The patch also adds a simple test-case for this issue. I think that the >>> ChangeLog entry should be something along the lines of: >>> >>> 2014-12-24 Dimitris Papavasiliou <dpapavas@gmail.com> >>> >>> PR libobjc/51891 >>> * libobjc/ivars.c: Add a check for classes without instance >>> variables, which have a NULL ivar list pointer. >>> * gcc/testsuite/objc.dg/gnu-api-2-class.m: Add a test case >>> for the above change. >>> >>> I hope I got the formatting right. I've run make -k check-objc and all >>> tests pass without problems. >>> >>> Let me know if there are any problems, or if I can do anything else to >>> facilitate the acceptance of the patch. >>> >>> Regards, >>> Dimitris >> >> >> So, Andrew is the reviewer for libobjc. I’m not. I don’t have any issue >> with it. > > Do you want to be a reviewer for libobjc? I don't think the load there is > high, but having someone else who cares about the code is always a good > thing. I am a reviewer for libobjc, I had missed this patch when it came in due to the low traffic of libobjc patches and working on other projects at the time. I had marked as something I needed to review but I did not get around to it until now. Thanks, Andrew > > jeff
On Jan 8, 2015, at 9:38 PM, Andrew Pinski <pinskia@gmail.com> wrote: >> 2014-12-24 Dimitris Papavasiliou <dpapavas@gmail.com> >> >> PR libobjc/51891 >> * libobjc/ivars.c: Add a check for classes without instance >> variables, which have a NULL ivar list pointer. >> * gcc/testsuite/objc.dg/gnu-api-2-class.m: Add a test case >> for the above change. > This is ok. Committed revision 219396.
On Jan 8, 2015, at 9:35 PM, Jeff Law <law@redhat.com> wrote:
> Do you want to be a reviewer for libobjc?
I think things are fine as is. If things were pinged and there were no response, or if someone wanted to do major updated on the library to bring it up a decade, then we might want to change things. If someone wanted to do a major update, then, I’d recommend they step forward.
Index: gcc/testsuite/objc.dg/gnu-api-2-class.m =================================================================== --- gcc/testsuite/objc.dg/gnu-api-2-class.m (revision 219054) +++ gcc/testsuite/objc.dg/gnu-api-2-class.m (working copy) @@ -239,6 +239,19 @@ abort (); } + printf ("Testing class_copyIvarList () on class with no instance variables...\n"); + { + unsigned int count; + Ivar * list = class_copyIvarList (objc_getClass ("MyOtherSubClass"), + &count); + + if (count != 0) + abort (); + + if (list != NULL) + abort (); + } + printf ("Testing class_copyMethodList ()...\n"); { unsigned int count; Index: libobjc/ivars.c =================================================================== --- libobjc/ivars.c (revision 219054) +++ libobjc/ivars.c (working copy) @@ -179,7 +179,7 @@ struct objc_ivar **returnValue = NULL; struct objc_ivar_list* ivar_list; - if (class_ == Nil || CLS_IS_IN_CONSTRUCTION (class_)) + if (class_ == Nil || CLS_IS_IN_CONSTRUCTION (class_) || !class_->ivars) { if (numberOfReturnedIvars) *numberOfReturnedIvars = 0;
Hello, The attached patch fixes an issue reported a couple of years ago in Bug 51891 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51891). The problem is caused because classes without instance variables have no ivar list at all, so that their ivars pointer is NULL, but the code in class_copyIvarList () is unaware of this. That this is in fact so can be easily verified by checking the code of class_addIvar in the same source file, where the ivars list is allocated when the first ivar is added. The code there also checks for a NULL ivars pointer. The patch also adds a simple test-case for this issue. I think that the ChangeLog entry should be something along the lines of: 2014-12-24 Dimitris Papavasiliou <dpapavas@gmail.com> PR libobjc/51891 * libobjc/ivars.c: Add a check for classes without instance variables, which have a NULL ivar list pointer. * gcc/testsuite/objc.dg/gnu-api-2-class.m: Add a test case for the above change. I hope I got the formatting right. I've run make -k check-objc and all tests pass without problems. Let me know if there are any problems, or if I can do anything else to facilitate the acceptance of the patch. Regards, Dimitris