Patchwork =?UTF-8?Q?ObjC:=20fix=20PR=20objc/47784=20("Internal=20compiler=20error?= =?UTF-8?Q?=20in=20dot=20notation=20assignment=20of=20const=20value")?=

login
register
mail settings
Submitter Nicola Pero
Date Feb. 19, 2011, 5:08 a.m.
Message ID <1298092096.177523530@192.168.4.58>
Download mbox | patch
Permalink /patch/83685/
State New
Headers show

Comments

Nicola Pero - Feb. 19, 2011, 5:08 a.m.
This patch fixes objc/47784 ("Internal compiler error in dot notation
assignment of const value").  Due to an ingenuity in our implementation
of the dot-syntax,

 x.property = y;

would crash the ObjC compiler if the type of 'y' is some sort of 'const'
type (the ObjC++ would just refuse to compile it with an error).

This patch fixes it.  It seems worthwhile to apply it to 4.6 as the bug
is pretty bad; perfectly valid (and common) code would otherwise crash
GCC 4.6.

Ok to commit ?

Thanks
Mike Stump - Feb. 20, 2011, 4:55 p.m.
On Feb 18, 2011, at 9:08 PM, Nicola Pero wrote:
> This patch fixes objc/47784 ("Internal compiler error in dot notation
> assignment of const value").  Due to an ingenuity in our implementation
> of the dot-syntax,
> 
> x.property = y;
> 
> would crash the ObjC compiler if the type of 'y' is some sort of 'const'
> type (the ObjC++ would just refuse to compile it with an error).
> 
> This patch fixes it.  It seems worthwhile to apply it to 4.6 as the bug
> is pretty bad; perfectly valid (and common) code would otherwise crash
> GCC 4.6.
> 
> Ok to commit ?

Ok.

Patch

Index: objc/ChangeLog
===================================================================
--- objc/ChangeLog      (revision 170299)
+++ objc/ChangeLog      (working copy)
@@ -1,5 +1,11 @@ 
 2011-01-19  Nicola Pero  <nicola.pero@meta-innovation.com>
 
+       PR objc/47784
+       * objc-act.c (objc_maybe_build_modify_expr): If 'rhs' has side
+       effects, do not use a temporary variable.
+
+2011-01-19  Nicola Pero  <nicola.pero@meta-innovation.com>
+
        * objc-act.c (objc_init, generate_struct_by_value_array): Updated
        comments.
 
Index: objc/objc-act.c
===================================================================
--- objc/objc-act.c     (revision 170299)
+++ objc/objc-act.c     (working copy)
@@ -1790,49 +1790,71 @@  objc_maybe_build_modify_expr (tree lhs, tree rhs)
       to get these to work with very little effort, we build a
       compound statement which does the setter call (to set the
       property to 'rhs'), but which can also be evaluated returning
-      the 'rhs'.  So, we want to create the following:
+      the 'rhs'.  If the 'rhs' has no side effects, we can simply
+      evaluate it twice, building
 
-      (temp = rhs; [object setProperty: temp]; temp)
+      ([object setProperty: rhs]; rhs)
+
+      If it has side effects, we put it in a temporary variable first,
+      so we create the following:
+
+      (temp = rhs; [object setProperty: temp]; temp)
+
+      setter_argument is rhs in the first case, and temp in the second
+      case.
       */
-      tree temp_variable_decl, bind;
+      tree setter_argument;
+
       /* s1, s2 and s3 are the tree statements that we need in the
         compound expression.  */
       tree s1, s2, s3, compound_expr;
+
+      if (TREE_SIDE_EFFECTS (rhs))
+       {
+         tree bind;
       
-      /* TODO: If 'rhs' is a constant, we could maybe do without the
-        'temp' variable ? */
+         /* Declare __objc_property_temp in a local bind.  */
+         setter_argument = objc_create_temporary_var (TREE_TYPE (rhs), "__objc_property_temp");
+         DECL_SOURCE_LOCATION (setter_argument) = input_location;
+         bind = build3 (BIND_EXPR, void_type_node, setter_argument, NULL, NULL);
+         SET_EXPR_LOCATION (bind, input_location);
+         TREE_SIDE_EFFECTS (bind) = 1;
+         add_stmt (bind);
 
-      /* Declare __objc_property_temp in a local bind.  */
-      temp_variable_decl = objc_create_temporary_var (TREE_TYPE (rhs), "__objc_property_temp");
-      DECL_SOURCE_LOCATION (temp_variable_decl) = input_location;
-      bind = build3 (BIND_EXPR, void_type_node, temp_variable_decl, NULL, NULL);
-      SET_EXPR_LOCATION (bind, input_location);
-      TREE_SIDE_EFFECTS (bind) = 1;
-      add_stmt (bind);
+         /* s1: x = rhs */
+         s1 = build_modify_expr (input_location, setter_argument, NULL_TREE,
+                                 NOP_EXPR,
+                                 input_location, rhs, NULL_TREE);
+         SET_EXPR_LOCATION (s1, input_location);
+       }
+      else
+       {
+         /* No s1.  */
+         setter_argument = rhs;
+         s1 = NULL_TREE;
+       }
       
       /* Now build the compound statement.  */
+  
+      /* s2: [object setProperty: x] */
+      s2 = objc_build_setter_call (lhs, setter_argument);
       
-      /* s1: __objc_property_temp = rhs */
-      s1 = build_modify_expr (input_location, temp_variable_decl, NULL_TREE,
-                             NOP_EXPR,
-                             input_location, rhs, NULL_TREE);
-      SET_EXPR_LOCATION (s1, input_location);
-  
-      /* s2: [object setProperty: __objc_property_temp] */
-      s2 = objc_build_setter_call (lhs, temp_variable_decl);
-
-      /* This happens if building the setter failed because the property
-        is readonly.  */
+      /* This happens if building the setter failed because the
+        property is readonly.  */
       if (s2 == error_mark_node)
        return error_mark_node;
 
       SET_EXPR_LOCATION (s2, input_location);
   
-      /* s3: __objc_property_temp */
-      s3 = convert (TREE_TYPE (lhs), temp_variable_decl);
+      /* s3: x */
+      s3 = convert (TREE_TYPE (lhs), setter_argument);
 
-      /* Now build the compound statement (s1, s2, s3) */
-      compound_expr = build_compound_expr (input_location, build_compound_expr (input_location, s1, s2), s3);
+      /* Now build the compound statement (s1, s2, s3) or (s2, s3) as
+        appropriate.  */
+      if (s1)
+       compound_expr = build_compound_expr (input_location, build_compound_expr (input_location, s1, s2), s3);
+      else
+       compound_expr = build_compound_expr (input_location, s2, s3);   
 
       /* Without this, with -Wall you get a 'valued computed is not
         used' every time there is a "object.property = x" where the
Index: testsuite/ChangeLog
===================================================================
--- testsuite/ChangeLog (revision 170297)
+++ testsuite/ChangeLog (working copy)
@@ -1,3 +1,9 @@ 
+2011-02-19  Nicola Pero  <nicola.pero@meta-innovation.com>
+
+       PR objc/47784
+       * objc.dg/property/dotsyntax-22.m: New.
+       * obj-c++.dg/property/dotsyntax-22.mm: New.
+       
 2011-02-18  Iain Sandoe  <iains@gcc.gnu.org>
 
        * objc/execute/exceptions/foward-1.x: New.
Index: testsuite/objc.dg/property/dotsyntax-22.m
===================================================================
--- testsuite/objc.dg/property/dotsyntax-22.m   (revision 0)
+++ testsuite/objc.dg/property/dotsyntax-22.m   (revision 0)
@@ -0,0 +1,19 @@ 
+/* PR objc/47784.  This testcase used to crash the compiler.  */
+
+typedef struct {
+  float x;
+} SomeType;
+
+@interface MyClass
+
+@property(assign,readwrite) SomeType position;
+
+@end
+
+void example (MyClass *x)
+{
+  const SomeType SomeTypeZero = {0.0f};
+
+  x.position= SomeTypeZero;
+}
+
Index: testsuite/obj-c++.dg/property/dotsyntax-22.mm
===================================================================
--- testsuite/obj-c++.dg/property/dotsyntax-22.mm       (revision 0)
+++ testsuite/obj-c++.dg/property/dotsyntax-22.mm       (revision 0)
@@ -0,0 +1,19 @@ 
+/* PR objc/47784.  This testcase used to crash the compiler.  */
+
+typedef struct {
+  float x;
+} SomeType;
+
+@interface MyClass
+
+@property(assign,readwrite) SomeType position;
+
+@end
+
+void example (MyClass *x)
+{
+  const SomeType SomeTypeZero = {0.0f};
+
+  x.position= SomeTypeZero;
+}
+