diff mbox

ObjC/ObjC++: fix for classes with more than 15 instance variables

Message ID 1290712103.915816754@192.168.2.228
State New
Headers show

Commit Message

Nicola Pero Nov. 25, 2010, 7:08 p.m. UTC 
This patch fixes a bug in the Objective-C compiler that would cause
root classes with more than 15 instance variables to trigger spurious
warnings.

The patch includes a testcase that shows a spurious warning 
with a root class; that's how I found the bug, but looking at the
code it seems likely that you may get other spurious warnings with 
non-root classes if they have over 15 instance variables (I don't
have a testcase for that handy though).

The reason for the bug is that when the struct associated with the 
class is built, the C frontend generates an internal sorted list 
of the fields if there are more than 15 fields (that is where the
otherwise puzzling limit of 15 instance variables come from).  That 
sorting is then stored in lang_type, which is the same place where the ObjC
info for interfaces and protocols is stored, and storing the sorting
info wipes the Objc info out.

The ObjC frontend already had code to restore the protocol info associated
with type variants.  Unfortunately, it didn't have code to restore 
the actual interface that the new struct is associated with, so it would 
no longer be associated with it, and you'd get spurious warnings 
when trying to use it as the type-checking would no longer work ;-)

This patch fixes it, and includes testcases.

Ok to commit ?

Thanks

PS: I think the whole lang_type/objc_info thing should be revisited, but 
that's not for stage 3.  This patch simply fits in the existing framework
and fixes the immediate bug.

Comments

Mike Stump Nov. 25, 2010, 7:21 p.m. UTC | #1 
On Nov 25, 2010, at 11:08 AM, Nicola Pero wrote:
> This patch fixes a bug in the Objective-C compiler that would cause
> root classes with more than 15 instance variables to trigger spurious
> warnings.

> Ok to commit ?

Ok.
diff mbox

Patch

Index: objc/objc-act.c
===================================================================
--- objc/objc-act.c     (revision 167099)
+++ objc/objc-act.c     (working copy)
@@ -2137,10 +2137,15 @@  objc_build_struct (tree klass, tree fields, tree s
       fields = base;
     }
 
-  /* NB: Calling finish_struct() may cause type TYPE_LANG_SPECIFIC fields
-     in all variants of this RECORD_TYPE to be clobbered, but it is therein
-     that we store protocol conformance info (e.g., 'NSObject <MyProtocol>').
-     Hence, we must squirrel away the ObjC-specific information before calling
+  /* NB: Calling finish_struct() may cause type TYPE_LANG_SPECIFIC
+     fields in all variants of this RECORD_TYPE to be clobbered (this
+     is because the C frontend stores a sorted version of the list of
+     fields in lang_type if it deems appropriate, and will update and
+     propagate that list to all variants ignoring the fact that we use
+     lang_type for something else and that such propagation will wipe
+     the objc_info away), but it is therein that we store protocol
+     conformance info (e.g., 'NSObject <MyProtocol>').  Hence, we must
+     squirrel away the ObjC-specific information before calling
      finish_struct(), and then reinstate it afterwards.  */
 
   for (t = TYPE_NEXT_VARIANT (s); t; t = TYPE_NEXT_VARIANT (t))
@@ -2153,12 +2158,16 @@  objc_build_struct (tree klass, tree fields, tree s
       VEC_safe_push (tree, heap, objc_info, TYPE_OBJC_INFO (t));
     }
 
-  /* Point the struct at its related Objective-C class.  */
-  INIT_TYPE_OBJC_INFO (s);
+  s = objc_finish_struct (s, fields);
+
+  /* Point the struct at its related Objective-C class.  We do this
+     after calling finish_struct() because otherwise finish_struct()
+     would wipe TYPE_OBJC_INTERFACE() out.  */
+  if (!TYPE_HAS_OBJC_INFO (s))
+    INIT_TYPE_OBJC_INFO (s);
+
   TYPE_OBJC_INTERFACE (s) = klass;
 
-  s = objc_finish_struct (s, fields);
-
   for (i = 0, t = TYPE_NEXT_VARIANT (s); t; t = TYPE_NEXT_VARIANT (t), i++)
     {
       TYPE_OBJC_INFO (t) = VEC_index (tree, objc_info, i);
Index: objc/ChangeLog
===================================================================
--- objc/ChangeLog      (revision 167099)
+++ objc/ChangeLog      (working copy)
@@ -1,3 +1,10 @@ 
+2010-11-25  Nicola Pero  <nicola.pero@meta-innovation.com>
+
+       * objc-act.c (objc_build_struct): Install TYPE_OBJC_INTERFACE
+       after finish_struct, not before, otherwise it may be wiped out by
+       it.  This fixes spurious warnings when a class has more than 15
+       instance variables.
+
 2010-11-23  Nicola Pero  <nicola.pero@meta-innovation.com>
 
        PR objc/24358
Index: testsuite/ChangeLog
===================================================================
--- testsuite/ChangeLog (revision 167099)
+++ testsuite/ChangeLog (working copy)
@@ -1,3 +1,8 @@ 
+2010-11-25  Nicola Pero  <nicola.pero@meta-innovation.com>
+
+       * objc.dg/ivar-problem-1.m: New.
+       * obj-c++.dg/ivar-problem-1.mm: New.
+
 2010-11-23  Iain Sandoe  <iains@gcc.gnu.org>
 
        * gcc.dg/darwin-cfstring-1.c: Adjust format messages.
Index: testsuite/objc.dg/ivar-problem-1.m
===================================================================
--- testsuite/objc.dg/ivar-problem-1.m  (revision 0)
+++ testsuite/objc.dg/ivar-problem-1.m  (revision 0)
@@ -0,0 +1,65 @@ 
+/* Contributed by Nicola Pero <nicola.pero@meta-innovation.com>, November 2010.  */
+/* { dg-do compile } */
+
+/* This test checks what happens if there are 16 instance variables.
+   In that case, the class was not created correctly.  In this testcase,
+   we have two classes, one with 15 variables and one with 16.  Older
+   GCCs would generate a bogus warning for the second class but not 
+   for the first one.  */
+
+#include <stdlib.h>
+#include <objc/objc.h>
+
+@interface MyRootClass1
+{
+  Class isa;
+  int v2;
+  int v3;
+  int v4;
+  int v5;
+  int v6;
+  int v7;
+  int v8;
+  int v9;
+  int v10;
+  int v11;
+  int v12;
+  int v13;
+  int v14;
+  int v15;
+}
+- (id) init;
+@end
+
+@implementation MyRootClass1
+- (id) init { return self; }
+@end
+
+
+@interface MyRootClass2
+{
+  Class isa;
+  int v2;
+  int v3;
+  int v4;
+  int v5;
+  int v6;
+  int v7;
+  int v8;
+  int v9;
+  int v10;
+  int v11;
+  int v12;
+  int v13;
+  int v14;
+  int v15;
+  /* Adding the 16th variable used to cause bogus warnings to be
+     generated.  */
+  int v16;
+}
+- (id) init;
+@end
+
+@implementation MyRootClass2
+- (id) init { return self; } /* This should not generate a bogus warning.  */
+@end
Index: testsuite/obj-c++.dg/ivar-problem-1.mm
===================================================================
--- testsuite/obj-c++.dg/ivar-problem-1.mm      (revision 0)
+++ testsuite/obj-c++.dg/ivar-problem-1.mm      (revision 0)
@@ -0,0 +1,66 @@ 
+/* Contributed by Nicola Pero <nicola.pero@meta-innovation.com>, November 2010.  */
+/* { dg-do compile } */
+
+/* This test checks what happens if there are 16 instance variables.
+   In that case, the class was not created correctly.  In this testcase,
+   we have two classes, one with 15 variables and one with 16.  Older
+   GCCs would generate a bogus warning for the second class but not 
+   for the first one.  This only happened for ObjC, but it's good to
+   test ObjC++ as well.  */
+
+#include <stdlib.h>
+#include <objc/objc.h>
+
+@interface MyRootClass1
+{
+  Class isa;
+  int v2;
+  int v3;
+  int v4;
+  int v5;
+  int v6;
+  int v7;
+  int v8;
+  int v9;
+  int v10;
+  int v11;
+  int v12;
+  int v13;
+  int v14;
+  int v15;
+}
+- (id) init;
+@end
+
+@implementation MyRootClass1
+- (id) init { return self; }
+@end
+
+
+@interface MyRootClass2
+{
+  Class isa;
+  int v2;
+  int v3;
+  int v4;
+  int v5;
+  int v6;
+  int v7;
+  int v8;
+  int v9;
+  int v10;
+  int v11;
+  int v12;
+  int v13;
+  int v14;
+  int v15;
+  /* Adding the 16th variable used to cause bogus warnings to be
+     generated.  */
+  int v16;
+}
+- (id) init;
+@end
+
+@implementation MyRootClass2
+- (id) init { return self; } /* This should not generate a bogus warning.  */
+@end