From patchwork Mon Feb 21 19:30:32 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nicola Pero X-Patchwork-Id: 83868 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 18C5DB7181 for ; Tue, 22 Feb 2011 06:30:46 +1100 (EST) Received: (qmail 23164 invoked by alias); 21 Feb 2011 19:30:44 -0000 Received: (qmail 23149 invoked by uid 22791); 21 Feb 2011 19:30:42 -0000 X-SWARE-Spam-Status: No, hits=-1.3 required=5.0 tests=AWL, BAYES_00, SARE_SUB_ENC_UTF8, TW_BJ, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from fencepost.gnu.org (HELO fencepost.gnu.org) (140.186.70.10) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 21 Feb 2011 19:30:37 +0000 Received: from eggs.gnu.org ([140.186.70.92]:60929) by fencepost.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1PrbSt-00077V-IL for gcc-patches@gnu.org; Mon, 21 Feb 2011 14:30:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PrbSr-0006ku-C7 for gcc-patches@gnu.org; Mon, 21 Feb 2011 14:30:35 -0500 Received: from smtp111.iad.emailsrvr.com ([207.97.245.111]:44292) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PrbSr-0006kE-8r for gcc-patches@gnu.org; Mon, 21 Feb 2011 14:30:33 -0500 Received: from localhost (localhost.localdomain [127.0.0.1]) by smtp41.relay.iad1a.emailsrvr.com (SMTP Server) with ESMTP id 3C42329167A for ; Mon, 21 Feb 2011 14:30:32 -0500 (EST) Received: from dynamic13.wm-web.iad.mlsrvr.com (dynamic13.wm-web.iad1a.rsapps.net [192.168.2.220]) by smtp41.relay.iad1a.emailsrvr.com (SMTP Server) with ESMTP id 2B2C42914EC for ; Mon, 21 Feb 2011 14:30:32 -0500 (EST) Received: from meta-innovation.com (localhost [127.0.0.1]) by dynamic13.wm-web.iad.mlsrvr.com (Postfix) with ESMTP id 13CC53218001 for ; Mon, 21 Feb 2011 14:30:32 -0500 (EST) Received: by www2.webmail.us (Authenticated sender: nicola.pero@meta-innovation.com, from: nicola.pero@meta-innovation.com) with HTTP; Mon, 21 Feb 2011 20:30:32 +0100 (CET) Date: Mon, 21 Feb 2011 20:30:32 +0100 (CET) Subject: =?UTF-8?Q?Fix=20for=20PR=20objc/47832=20("[4.6=20Regression]=20ObjC=20er?= =?UTF-8?Q?rors=20on=20structures=20with=20flexible=20data=20members")?= From: "Nicola Pero" To: "gcc-patches@gnu.org" MIME-Version: 1.0 X-Type: plain Message-ID: <1298316632.079627451@192.168.4.58> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-Received-From: 207.97.245.111 X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org GCC 4.6 tries to do a better job at catching invalid types used in instance variables; in particular, it tries to catch structures with flexible data members. Ie, struct A { int x; int[] y; } @interface B { struct A z; } @end should be rejected. Unfortunately, the check caused a regression (objc/47832) because of its lack of sophistication. The check worked by detecting when an array of unknown size was being encoded as part of the generation of instance variables. That's all good, except that such encoding can happen even if a pointer to such a structure is used as instance variable, as in -- struct A { int x; int[] y; } @interface B { struct A *z; } @end which is perfectly valid code, but was rejected too (this is exactly the regression described in PR objc/47832). This patch fixes the problem by rearranging the check to happen earlier, when the instance variable is added. As an added bonus, we can print the error on the correct line, and mention the invalid instance variable too :-) This patch was written for "stage 4", hence it tries to be as robust and localized as possible. I don't want to touch the C frontend or anything else. I added ample comments to remind us that we can reorganize the code better for 4.7. I had ObjC++ skip the check altogether, so that there can't be any regressions for ObjC++, but also, obviously, there is no check either. Testcase included. Ok to commit ? Thanks Index: objc/ChangeLog =================================================================== --- objc/ChangeLog (revision 170370) +++ objc/ChangeLog (working copy) @@ -1,10 +1,19 @@ 2011-02-20 Nicola Pero - * objc-gnu-runtime-abi-01.c (TARGET_64BIT): Removed. Removed - usage of padding fields. Do not include tm.h. - * objc-act.c (objc_write_global_declaration): Set input_location - to BUILTINS_LOCATION while generating runtime metadata. + PR objc/47832 + * objc-act.c (flexible_array_type_p): New. + (add_instance_variable): Produce an error if an instance variable + is of flexible array type. + (encode_array): Do not emit an error if encoding a flexible array + type while generating instance variables. +2011-02-20 Nicola Pero + + * objc-gnu-runtime-abi-01.c (TARGET_64BIT): Removed. Removed + usage of padding fields. Do not include tm.h. + * objc-act.c (objc_write_global_declaration): Set input_location + to BUILTINS_LOCATION while generating runtime metadata. + 2011-01-20 Nicola Pero PR objc/47784 Index: objc/objc-act.c =================================================================== --- objc/objc-act.c (revision 170370) +++ objc/objc-act.c (working copy) @@ -5925,6 +5925,49 @@ add_category (tree klass, tree category) } } +/* TODO: The check for flexible array members in instance variables is + implemented only for ObjC, and it is implemented by using this + flexible_array_type_p() function, which is basically lifted from + c-decl.c. Is this the best way of doing the check ? */ + +/* flexible_array_type_p() seems to crash the compiler when used in + ObjC++, so we disable the check in ObjC++. */ +#ifndef OBJCPLUS +/* Determine whether TYPE is a structure with a flexible array member, + or a union containing such a structure (possibly recursively). These + are invalid as instance variable. */ + +static bool +flexible_array_type_p (tree type) +{ + tree x; + switch (TREE_CODE (type)) + { + case RECORD_TYPE: + x = TYPE_FIELDS (type); + if (x == NULL_TREE) + return false; + while (DECL_CHAIN (x) != NULL_TREE) + x = DECL_CHAIN (x); + if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE + && TYPE_SIZE (TREE_TYPE (x)) == NULL_TREE + && TYPE_DOMAIN (TREE_TYPE (x)) != NULL_TREE + && TYPE_MAX_VALUE (TYPE_DOMAIN (TREE_TYPE (x))) == NULL_TREE) + return true; + return false; + case UNION_TYPE: + for (x = TYPE_FIELDS (type); x != NULL_TREE; x = DECL_CHAIN (x)) + { + if (flexible_array_type_p (TREE_TYPE (x))) + return true; + } + return false; + default: + return false; + } +} +#endif + /* Called after parsing each instance variable declaration. Necessary to preserve typedefs and implement public/private... @@ -5958,6 +6001,27 @@ add_instance_variable (tree klass, objc_ivar_visib return klass; } + /* Also, reject a struct with a flexible array member. Ie, + + struct A { int x; int[] y; }; + + @interface X + { + struct A instance_variable; + } + @end + + is not valid as 'struct A' doesn't have a known size. */ +#ifndef OBJCPLUS + /* TODO: Implemented this check for ObjC++ too. */ + if (flexible_array_type_p (field_type)) + { + error ("instance variable %qs has unknown size", ivar_name); + /* Return class as is without adding this ivar. */ + return klass; + } +#endif + #ifdef OBJCPLUS /* Check if the ivar being added has a non-POD C++ type. If so, we will need to either (1) warn the user about it or (2) generate suitable @@ -9926,27 +9990,23 @@ encode_array (tree type, int curtype, int format) if (an_int_cst == NULL) { /* We are trying to encode an incomplete array. An incomplete - array is forbidden as part of an instance variable. */ - if (generating_instance_variables) - { - /* TODO: Detect this error earlier. */ - error ("instance variable has unknown size"); - return; - } + array is forbidden as part of an instance variable; but it + may occur if the instance variable is a pointer to such an + array. */ - /* So the only case in which an incomplete array could occur is - if we are encoding the arguments or return value of a method. - In that case, an incomplete array argument or return value - (eg, -(void)display: (char[])string) is treated like a - pointer because that is how the compiler does the function - call. A special, more complicated case, is when the - incomplete array is the last member of a struct (eg, if we - are encoding "struct { unsigned long int a;double b[];}"), - which is again part of a method argument/return value. In - that case, we really need to communicate to the runtime that - there is an incomplete array (not a pointer!) there. So, we - detect that special case and encode it as a zero-length - array. + /* So the only case in which an incomplete array could occur + (without being pointed to) is if we are encoding the + arguments or return value of a method. In that case, an + incomplete array argument or return value (eg, + -(void)display: (char[])string) is treated like a pointer + because that is how the compiler does the function call. A + special, more complicated case, is when the incomplete array + is the last member of a struct (eg, if we are encoding + "struct { unsigned long int a;double b[];}"), which is again + part of a method argument/return value. In that case, we + really need to communicate to the runtime that there is an + incomplete array (not a pointer!) there. So, we detect that + special case and encode it as a zero-length array. Try to detect that we are part of a struct. We do this by searching for '=' in the type encoding for the current type. Index: testsuite/ChangeLog =================================================================== --- testsuite/ChangeLog (revision 170370) +++ testsuite/ChangeLog (working copy) @@ -1,3 +1,8 @@ +2011-02-21 Nicola Pero + + PR objc/47832 + * objc.dg/type-size-4.m: New test. + 2011-02-21 Jeff Law PR rtl-optimization/46178 Index: testsuite/objc.dg/type-size-3.m =================================================================== --- testsuite/objc.dg/type-size-3.m (revision 170370) +++ testsuite/objc.dg/type-size-3.m (working copy) @@ -10,11 +10,9 @@ typedef struct @interface Test { - test_type c; + test_type c; /* { dg-error "instance variable .c. has unknown size" } */ } @end @implementation Test @end - -/* { dg-error "instance variable has unknown size" "" { target *-*-* } 0 } */ Index: testsuite/objc.dg/type-size-4.m =================================================================== --- testsuite/objc.dg/type-size-4.m (revision 0) +++ testsuite/objc.dg/type-size-4.m (revision 0) @@ -0,0 +1,20 @@ +/* Allow ivars that are pointers to structs with an unknown size. */ +/* Contributed by Nicola Pero */ +/* PR objc/47832 */ +/* { dg-do compile } */ + +typedef struct +{ + unsigned long int a; + double b[]; +} test_type; + +@interface Test +{ + /* This is fine. */ + test_type *c; +} +@end + +@implementation Test +@end