ObjC/ObjC++: Yet another protocol type-checking fix
IainS
developer@sandoe-acoustics.co.uk
Sat Nov 27 00:22:00 GMT 2010
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];
> +}
>
>
>
More information about the Gcc-patches
mailing list