ObjC/ObjC++: Yet another protocol type-checking fix

This patch fixes yet another problem with ObjC type checking.

The problem is in the same spot of code that I patched yesterday (but it's
a completely different one), and occurs when a class is forward-declared 
using @class, used in some declarations that involve protocols, and then 
later the appropriate @interface declaration is found.

When the @interface declaration is found, the C struct corresponding to the class 
is built, which calls finish_struct() which messes up TYPE_LANG_SPECIFIC for all
type variants (ie, all the ones that have been created, in the meanwhile, using
the class forward declaration), in particular any associated protocol qualifier 
information.

The ObjC frontend had code to restore this after finish_struct(), but upon further 
inspection, and studying a failing testcase (included in this patch) it seems that
the existing code wasn't good enough (or maybe had been good enough a long time ago);
the reason being that the C frontend does not just copy TYPE_LANG_SPECIFIC from
one variant to the other, but, I guess for efficiency reasons, it actually
makes them all *the same* TYPE_LANG_SPECIFIC.  The result is that the code in the
ObjC frontend trying to restore the different TYPE_OBJC_INFO for each type
would fail, simply because restoring TYPE_OBJC_INFO on one type would actually
implicitly change it for any other type as well ... so the last one being restored
would actually be applied to all types, and the ObjC protocol information would be
"clobbered" for all types all the same.  This patch fixes it by duplicating
TYPE_LANG_SPECIFIC in each type before patching it with the original TYPE_OBJC_INFO 
(in the spirit of stage 3, this patch doesn't attempt to reorganize how protocol
qualifiers are associated with types in ObjC; it just fixes the bug in the existing 
framework).

The patch includes:

 * a fix for objc-act.c

 * a testcase (protocol-qualifier-2.m) that is fixed by the patch

 * a testcase (protocol-qualifier-1.m) that was already fixed but that fails on
older compilers and which I'd like to include as a generally useful testcase

 * variants of the testcases for ObjC++; these work before and after the patch,
but as usual it's good to test them with the ObjC++ compiler as well.

Ok to commit ?

Thanks

On 26 Nov 2010, at 18:47, Nicola Pero wrote:

> This patch fixes yet another problem with ObjC type checking.
>
> The problem is in the same spot of code that I patched yesterday  
> (but it's
> a completely different one), and occurs when a class is forward- 
> declared
> using @class, used in some declarations that involve protocols, and  
> then
> later the appropriate @interface declaration is found.
>
> When the @interface declaration is found, the C struct corresponding  
> to the class
> is built, which calls finish_struct() which messes up  
> TYPE_LANG_SPECIFIC for all
> type variants (ie, all the ones that have been created, in the  
> meanwhile, using
> the class forward declaration), in particular any associated  
> protocol qualifier
> information.
>
> The ObjC frontend had code to restore this after finish_struct(),  
> but upon further
> inspection, and studying a failing testcase (included in this patch)  
> it seems that
> the existing code wasn't good enough (or maybe had been good enough  
> a long time ago);
> the reason being that the C frontend does not just copy  
> TYPE_LANG_SPECIFIC from
> one variant to the other, but, I guess for efficiency reasons, it  
> actually
> makes them all *the same* TYPE_LANG_SPECIFIC.  The result is that  
> the code in the
> ObjC frontend trying to restore the different TYPE_OBJC_INFO for  
> each type
> would fail, simply because restoring TYPE_OBJC_INFO on one type  
> would actually
> implicitly change it for any other type as well ... so the last one  
> being restored
> would actually be applied to all types, and the ObjC protocol  
> information would be
> "clobbered" for all types all the same.  This patch fixes it by  
> duplicating
> TYPE_LANG_SPECIFIC in each type before patching it with the original  
> TYPE_OBJC_INFO
> (in the spirit of stage 3, this patch doesn't attempt to reorganize  
> how protocol
> qualifiers are associated with types in ObjC; it just fixes the bug  
> in the existing
> framework).

just out of curiosity..
...  - would it be an alternative to save the whole TYPE_LANG_SPECIFIC  
entry across the finish_struct() call?

>
> The patch includes:
>
> * a fix for objc-act.c
>
> * a testcase (protocol-qualifier-2.m) that is fixed by the patch
>
> * a testcase (protocol-qualifier-1.m) that was already fixed but  
> that fails on
> older compilers and which I'd like to include as a generally useful  
> testcase
>
> * variants of the testcases for ObjC++; these work before and after  
> the patch,
> but as usual it's good to test them with the ObjC++ compiler as well.
>
> Ok to commit ?
>
> Thanks
>
> Index: gcc/objc/objc-act.c
> ===================================================================
> --- gcc/objc/objc-act.c (revision 167151)
> +++ gcc/objc/objc-act.c (working copy)
> @@ -2137,41 +2137,54 @@ 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 (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
> +  /* NB: Calling finish_struct() may cause type TYPE_OBJC_INFO
> +     information in all variants of this RECORD_TYPE to be destroyed
> +     (this is because the C frontend manipulates TYPE_LANG_SPECIFIC
> +     for something else and then will change all variants to use the
> +     same resulting TYPE_LANG_SPECIFIC, ignoring the fact that we use
> +     it for ObjC protocols and that such propagation will make all
> +     variants use the same objc_info), but it is therein that we  
> store
> +     protocol conformance info (e.g., 'NSObject <MyProtocol>').
> +     Hence, we must save 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))
> +  for (t = TYPE_MAIN_VARIANT (s); t; t = TYPE_NEXT_VARIANT (t))
>     {
> -      if (!TYPE_HAS_OBJC_INFO (t))
> -       {
> -         INIT_TYPE_OBJC_INFO (t);
> -         TYPE_OBJC_INTERFACE (t) = klass;
> -       }
> +      INIT_TYPE_OBJC_INFO (t);
>       VEC_safe_push (tree, heap, objc_info, TYPE_OBJC_INFO (t));
>     }
>
>   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;
> -
> -  for (i = 0, t = TYPE_NEXT_VARIANT (s); t; t = TYPE_NEXT_VARIANT  
> (t), i++)
> +  for (i = 0, t = TYPE_MAIN_VARIANT (s); t; t = TYPE_NEXT_VARIANT  
> (t), i++)
>     {
> +      /* We now want to restore the different TYPE_OBJC_INFO, but we
> +        have the additional problem that the C frontend doesn't just
> +        copy TYPE_LANG_SPECIFIC from one variant to the other; it
> +        actually makes all of them the *same* TYPE_LANG_SPECIFIC.  As
> +        we need a different TYPE_OBJC_INFO for each (and
> +        TYPE_OBJC_INFO is a field in TYPE_LANG_SPECIFIC), we need to
> +        make a copy of each TYPE_LANG_SPECIFIC before we modify
> +        TYPE_OBJC_INFO.  */
> +      if (TYPE_LANG_SPECIFIC (t))
> +       {
> +         /* Create a copy of TYPE_LANG_SPECIFIC.  */
> +         struct lang_type *old_lang_type = TYPE_LANG_SPECIFIC (t);
> +         ALLOC_OBJC_TYPE_LANG_SPECIFIC (t);
> +         memcpy (TYPE_LANG_SPECIFIC (t), old_lang_type,
> +                 SIZEOF_OBJC_TYPE_LANG_SPECIFIC);
> +       }
> +      else
> +       {
> +         /* Just create a new one.  */
> +         ALLOC_OBJC_TYPE_LANG_SPECIFIC (t);
> +       }
> +      /* Replace TYPE_OBJC_INFO with the saved one.  This restores  
> any
> +        protocol information that may have been associated with the
> +        type.  */
>       TYPE_OBJC_INFO (t) = VEC_index (tree, objc_info, i);
> -      /* Replace the IDENTIFIER_NODE with an actual @interface.  */
> +      /* Replace the IDENTIFIER_NODE with an actual @interface now
> +        that we have it.  */
>       TYPE_OBJC_INTERFACE (t) = klass;
>     }
>   VEC_free (tree, heap, objc_info);
> @@ -2766,9 +2779,12 @@ objc_non_volatilized_type (tree type)
>   return type;
> }
>
> -/* Construct a PROTOCOLS-qualified variant of INTERFACE, where  
> INTERFACE may
> -   either name an Objective-C class, or refer to the special 'id'  
> or 'Class'
> -   types.  If INTERFACE is not a valid ObjC type, just return it  
> unchanged.  */
> +/* Construct a PROTOCOLS-qualified variant of INTERFACE, where
> +   INTERFACE may either name an Objective-C class, or refer to the
> +   special 'id' or 'Class' types.  If INTERFACE is not a valid ObjC
> +   type, just return it unchanged.  This function is often called  
> when
> +   PROTOCOLS is NULL_TREE, in which case we simply look up the
> +   appropriate INTERFACE.  */
>
> tree
> objc_get_protocol_qualified_type (tree interface, tree protocols)
> @@ -4422,6 +4438,9 @@ objc_declare_class (tree ident_list)
>
>          record = xref_tag (RECORD_TYPE, ident);
>          INIT_TYPE_OBJC_INFO (record);
> +         /* In the case of a @class declaration, we store the ident
> +            in the TYPE_OBJC_INTERFACE.  If later an @interface is
> +            found, we'll replace the ident with the interface.  */
>          TYPE_OBJC_INTERFACE (record) = ident;
>          hash_class_name_enter (cls_name_hash_list, ident, NULL_TREE);
>        }
> Index: gcc/objc/ChangeLog
> ===================================================================
> --- gcc/objc/ChangeLog  (revision 167151)
> +++ gcc/objc/ChangeLog  (working copy)
> @@ -1,3 +1,12 @@
> +2010-11-26  Nicola Pero  <nicola.pero@meta-innovation.com>
> +
> +       * objc-act.c (objc_build_struct): Fixed loops that save and
> +       restore TYPE_OBJC_INFO to iterate over all variants of the  
> type; a
> +       special case for the current type is then no longer required.
> +       Duplicate TYPE_LANG_SPECIFIC for each type before restoring
> +       TYPE_OBJC_INFO.
> +       (objc_get_protocol_qualified_type): Updated comments.
> +
> 2010-11-25  Nicola Pero  <nicola.pero@meta-innovation.com>
>
>        * objc-act.c (objc_build_struct): Install TYPE_OBJC_INTERFACE
> Index: gcc/testsuite/ChangeLog
> ===================================================================
> --- gcc/testsuite/ChangeLog     (revision 167151)
> +++ gcc/testsuite/ChangeLog     (working copy)
> @@ -1,3 +1,10 @@
> +2010-11-26  Nicola Pero  <nicola.pero@meta-innovation.com>
> +
> +       * objc.dg/protocol-qualifier-1.m: New.
> +       * objc.dg/protocol-qualifier-2.m: New.
> +       * obj-c++.dg/protocol-qualifier-1.mm: New.
> +       * obj-c++.dg/protocol-qualifier-2.mm: New.
> +
> 2010-11-25  Nicola Pero  <nicola.pero@meta-innovation.com>
>
>        * objc.dg/ivar-problem-1.m: New.
> Index: gcc/testsuite/objc.dg/protocol-qualifier-1.m
> ===================================================================
> --- gcc/testsuite/objc.dg/protocol-qualifier-1.m        (revision 0)
> +++ gcc/testsuite/objc.dg/protocol-qualifier-1.m        (revision 0)
> @@ -0,0 +1,33 @@
> +/* Contributed by Nicola Pero <nicola.pero@meta-innovation.com>,  
> November 2010.  */
> +/* { dg-do compile } */
> +
> +/* Test that protocol qualifiers work in the same way with @class  
> and @interface.  */
> +
> +#include <objc/objc.h>
> +
> +@protocol MyProtocol
> +- (void) method;
> +@end
> +
> +
> +/* This first snippet gives no warnings, which is correct as 'object'
> +   implements <MyProtocol> and hence responds to 'method'.  Note how
> +   the details of the class 'MyClass' are never used.  */
> +@interface MyClass
> +@end
> +
> +void test (MyClass <MyProtocol> *object)
> +{
> +  [object method];
> +}
> +
> +
> +/* This second snippet should behave identically.  'object' still  
> implements
> +   the same protocol and responds to 'method'.  The details of  
> MyClass or
> +   MyClass2 are irrelevant.  */
> +@class MyClass2;
> +
> +void test2 (MyClass2 <MyProtocol> *object)
> +{
> +  [object method];
> +}
> Index: gcc/testsuite/objc.dg/protocol-qualifier-2.m
> ===================================================================
> --- gcc/testsuite/objc.dg/protocol-qualifier-2.m        (revision 0)
> +++ gcc/testsuite/objc.dg/protocol-qualifier-2.m        (revision 0)
> @@ -0,0 +1,31 @@
> +/* Contributed by Nicola Pero <nicola.pero@meta-innovation.com>,  
> November 2010.  */
> +/* { dg-do compile } */
> +
> +/* Test that protocol qualifiers are maintained correctly when a
> +   @class is replaced by its @interface.  */
> +
> +#include <objc/objc.h>
> +
> +@protocol MyProtocol
> +- (void) method;
> +@end
> +
> +@class MyClass;
> +
> +static MyClass <MyProtocol> *object1;
> +static MyClass *object2;
> +
> +/* Declarating the @interface now will need to update all the  
> existing
> +   ObjC types referring to MyClass with the new information.  We need
> +   to test that protocol information is not lost in the process.  */
> +@interface MyClass
> +@end
> +
> +void test1 (void)
> +{
> +  [object1 method]; /* Ok */
> +  [object2 method]; /* { dg-warning ".MyClass. may not respond  
> to ..method." } */
> +                    /* { dg-warning "without a matching method"  
> "" { target *-*-* } 27 } */
> +                    /* { dg-warning "will be assumed to return"  
> "" { target *-*-* } 27 } */
> +                    /* { dg-warning "as arguments" "" { target *-*- 
> * } 27 } */
> +}
> Index: gcc/testsuite/obj-c++.dg/protocol-qualifier-2.mm
> ===================================================================
> --- gcc/testsuite/obj-c++.dg/protocol-qualifier-2.mm    (revision 0)
> +++ gcc/testsuite/obj-c++.dg/protocol-qualifier-2.mm    (revision 0)
> @@ -0,0 +1,31 @@
> +/* Contributed by Nicola Pero <nicola.pero@meta-innovation.com>,  
> November 2010.  */
> +/* { dg-do compile } */
> +
> +/* Test that protocol qualifiers are maintained correctly when a
> +   @class is replaced by its @interface.  */
> +
> +#include <objc/objc.h>
> +
> +@protocol MyProtocol
> +- (void) method;
> +@end
> +
> +@class MyClass;
> +
> +static MyClass <MyProtocol> *object1;
> +static MyClass *object2;
> +
> +/* Declarating the @interface now will need to update all the  
> existing
> +   ObjC types referring to MyClass with the new information.  We need
> +   to test that protocol information is not lost in the process.  */
> +@interface MyClass
> +@end
> +
> +void test1 (void)
> +{
> +  [object1 method]; /* Ok */
> +  [object2 method]; /* { dg-warning ".MyClass. may not respond  
> to ..method." } */
> +                    /* { dg-warning "without a matching method"  
> "" { target *-*-* } 27 } */
> +                    /* { dg-warning "will be assumed to return"  
> "" { target *-*-* } 27 } */
> +                    /* { dg-warning "as arguments" "" { target *-*- 
> * } 27 } */
> +}
> Index: gcc/testsuite/obj-c++.dg/protocol-qualifier-1.mm
> ===================================================================
> --- gcc/testsuite/obj-c++.dg/protocol-qualifier-1.mm    (revision 0)
> +++ gcc/testsuite/obj-c++.dg/protocol-qualifier-1.mm    (revision 0)
> @@ -0,0 +1,33 @@
> +/* Contributed by Nicola Pero <nicola.pero@meta-innovation.com>,  
> November 2010.  */
> +/* { dg-do compile } */
> +
> +/* Test that protocol qualifiers work in the same way with @class  
> and @interface.  */
> +
> +#include <objc/objc.h>
> +
> +@protocol MyProtocol
> +- (void) method;
> +@end
> +
> +
> +/* This first snippet gives no warnings, which is correct as 'object'
> +   implements <MyProtocol> and hence responds to 'method'.  Note how
> +   the details of the class 'MyClass' are never used.  */
> +@interface MyClass
> +@end
> +
> +void test (MyClass <MyProtocol> *object)
> +{
> +  [object method];
> +}
> +
> +
> +/* This second snippet should behave identically.  'object' still  
> implements
> +   the same protocol and responds to 'method'.  The details of  
> MyClass or
> +   MyClass2 are irrelevant.  */
> +@class MyClass2;
> +
> +void test2 (MyClass2 <MyProtocol> *object)
> +{
> +  [object method];
> +}
>
>
>

>>  This patch fixes it by duplicating
>> TYPE_LANG_SPECIFIC in each type before patching it with the original  
>> TYPE_OBJC_INFO
>> (in the spirit of stage 3, this patch doesn't attempt to reorganize  
>> how protocol
>> qualifiers are associated with types in ObjC; it just fixes the bug  
>> in the existing
>> framework).
>
> just out of curiosity..
> ...  - would it be an alternative to save the whole TYPE_LANG_SPECIFIC  
> entry across the finish_struct() call?

Well ... you wouldn't want to have the ObjC frontend overwrite
what the C frontend writes in TYPE_LANG_SPECIFIC.  The C frontend uses
it for its own purposes.  At the moment the main use is to store a sorted 
list of the fields to speed up the lookup when there are many.  If the ObjC 
frontend overwrites TYPE_LANG_SPECIFIC, you'd lose that.

I think we should rewrite the whole lot in a better way, but it's for 4.7.

Thanks

On Nov 26, 2010, at 10:47 AM, Nicola Pero wrote:
> This patch fixes yet another problem with ObjC type checking.

> Ok to commit ?

Ok.