From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15178 invoked by alias); 4 Oct 2010 19:47:13 -0000 Received: (qmail 15165 invoked by uid 22791); 4 Oct 2010 19:47:10 -0000 X-SWARE-Spam-Status: No, hits=-2.2 required=5.0 tests=AWL,BAYES_00,TW_BJ,TW_CP,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mtagate5.de.ibm.com (HELO mtagate5.de.ibm.com) (195.212.17.165) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 04 Oct 2010 19:47:00 +0000 Received: from d12nrmr1607.megacenter.de.ibm.com (d12nrmr1607.megacenter.de.ibm.com [9.149.167.49]) by mtagate5.de.ibm.com (8.13.1/8.13.1) with ESMTP id o94JkveW003368 for ; Mon, 4 Oct 2010 19:46:57 GMT Received: from d12av01.megacenter.de.ibm.com (d12av01.megacenter.de.ibm.com [9.149.165.212]) by d12nrmr1607.megacenter.de.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o94JkuvU3973152 for ; Mon, 4 Oct 2010 21:46:56 +0200 Received: from d12av01.megacenter.de.ibm.com (loopback [127.0.0.1]) by d12av01.megacenter.de.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id o94JkutX013351 for ; Mon, 4 Oct 2010 21:46:56 +0200 Received: from leonard.localnet (ICON-9-164-144-251.megacenter.de.ibm.com [9.164.144.251]) by d12av01.megacenter.de.ibm.com (8.12.11.20060308/8.12.11) with ESMTP id o94JkoVx013312 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 4 Oct 2010 21:46:56 +0200 From: Ken Werner To: "Ulrich Weigand" , gdb-patches@sourceware.org, Joel Brobecker Subject: Re: [patch] fix pre-/post- in-/decrement Date: Mon, 04 Oct 2010 19:47:00 -0000 User-Agent: KMail/1.13.2 (Linux/2.6.32-25-generic; KDE/4.4.2; i686; ; ) References: <201010041301.o94D1QHV032611@d12av02.megacenter.de.ibm.com> In-Reply-To: <201010041301.o94D1QHV032611@d12av02.megacenter.de.ibm.com> MIME-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_q8iqMYRhLSHI+g8" Message-Id: <201010042146.50252.ken@linux.vnet.ibm.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-10/txt/msg00029.txt.bz2 --Boundary-00=_q8iqMYRhLSHI+g8 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-length: 2259 On Monday, October 04, 2010 3:01:22 pm Ulrich Weigand wrote: > Ken Werner wrote: > > - return value_assign (arg1, arg2); > > + /* Prevent to return a lvalue. */ > > + arg3 = value_assign (arg1, arg2); > > + VALUE_LVAL (arg3) = not_lval; > > + return arg3; > > We want to get away from changing core properties like lval > in values after the fact ... In any case, hard-coding the > lval to non_lval without any further change can cause problems, > e.g. if the value is lazy. Thank you for looking into the patch and clarifying things. > I think there is a more general issue underlying this particular > change. You're right that the result of a preincrement should > not be an lvalue. But the same is true for results of assignment > operators in general. Interesting - fixed. : ) > Note that value_assign is used only to implement such operators > (simple assignment, compound assignment, pre/postfix operators). > Since *all* of them return non-lvalues, it might make sense to > simply change value_assign to return a non-lvalue in the first > place ... Sounds good because value_assign would match the C semantic then. Unfortunately the varobj.c:varobj_set_value function (through the gdb_value_assign wrapper) seems to rely on the return value of value_assing beeing a lvalue. Therefore I decided to adjust the places where value_assign is called. > > + type = value_type (arg1); > > + arg3 = allocate_value (type); > > + > > + /* Copy the value to prevent to return a lvalue. */ > > + memcpy (value_contents_raw (arg3), value_contents (arg1), > > + TYPE_LENGTH (type)); > > I'd prefer to encapsulate this in a function, e.g. value_non_lval (...) > or a similar name, which returns a version of the value that is non_lval. > This could then address a couple of additional issues: > - if the value is already non_lval, no need to create an extra copy > - it might be better to copy the full contents / enclosing type > for C++ objects The attached patch introduces a new function called value_non_lval that returns a non-lval version of the given value. This function is called prior the simple assignment, compound assignment and pre/postfix routines return. Any comments are appreciated. Regards Ken Werner --Boundary-00=_q8iqMYRhLSHI+g8 Content-Type: text/x-patch; charset="UTF-8"; name="pre_post_in_decrement.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="pre_post_in_decrement.patch" Content-length: 5347 ChangeLog: 2010-10-04 Ken Werner * value.h (value_non_lval): Declare. * value.c (value_non_lval): New function. * eval.c (evaluate_subexp_standard) : Call value_non_lval to ensure to return a non-lvalue. testsuite/ChangeLog: 2010-10-04 Ken Werner * gdb.base/exprs.exp: Add tests for pre-/post- in-/decrement operators. Index: gdb/eval.c =================================================================== RCS file: /cvs/src/src/gdb/eval.c,v retrieving revision 1.139 diff -p -u -r1.139 eval.c --- gdb/eval.c 11 Aug 2010 16:48:26 -0000 1.139 +++ gdb/eval.c 4 Oct 2010 18:11:09 -0000 @@ -2010,7 +2010,7 @@ evaluate_subexp_standard (struct type *e if (binop_user_defined_p (op, arg1, arg2)) return value_x_binop (arg1, arg2, op, OP_NULL, noside); else - return value_assign (arg1, arg2); + return value_non_lval (value_assign (arg1, arg2)); case BINOP_ASSIGN_MODIFY: (*pos) += 2; @@ -2043,7 +2043,7 @@ evaluate_subexp_standard (struct type *e arg2 = value_binop (tmp, arg2, op); } - return value_assign (arg1, arg2); + return value_non_lval (value_assign (arg1, arg2)); case BINOP_ADD: arg1 = evaluate_subexp_with_coercion (exp, pos, noside); @@ -2702,7 +2702,7 @@ evaluate_subexp_standard (struct type *e arg2 = value_binop (tmp, arg2, BINOP_ADD); } - return value_assign (arg1, arg2); + return value_non_lval (value_assign (arg1, arg2)); } case UNOP_PREDECREMENT: @@ -2726,7 +2726,7 @@ evaluate_subexp_standard (struct type *e arg2 = value_binop (tmp, arg2, BINOP_SUB); } - return value_assign (arg1, arg2); + return value_non_lval (value_assign (arg1, arg2)); } case UNOP_POSTINCREMENT: @@ -2739,6 +2739,8 @@ evaluate_subexp_standard (struct type *e } else { + arg3 = value_non_lval (arg1); + if (ptrmath_type_p (exp->language_defn, value_type (arg1))) arg2 = value_ptradd (arg1, 1); else @@ -2751,7 +2753,7 @@ evaluate_subexp_standard (struct type *e } value_assign (arg1, arg2); - return arg1; + return arg3; } case UNOP_POSTDECREMENT: @@ -2764,6 +2766,8 @@ evaluate_subexp_standard (struct type *e } else { + arg3 = value_non_lval (arg1); + if (ptrmath_type_p (exp->language_defn, value_type (arg1))) arg2 = value_ptradd (arg1, -1); else @@ -2776,7 +2780,7 @@ evaluate_subexp_standard (struct type *e } value_assign (arg1, arg2); - return arg1; + return arg3; } case OP_THIS: Index: gdb/value.c =================================================================== RCS file: /cvs/src/src/gdb/value.c,v retrieving revision 1.113 diff -p -u -r1.113 value.c --- gdb/value.c 30 Sep 2010 18:58:07 -0000 1.113 +++ gdb/value.c 4 Oct 2010 18:11:09 -0000 @@ -826,6 +826,26 @@ value_copy (struct value *arg) return val; } +/* Return a version of ARG that is non-lvalue. */ + +struct value * +value_non_lval (struct value *arg) +{ + if (VALUE_LVAL (arg) != not_lval) + { + struct type *enc_type = value_enclosing_type (arg); + struct value *val = allocate_value (enc_type); + + memcpy (value_contents_all_raw (val), value_contents_all (arg), + TYPE_LENGTH (enc_type)); + val->type = arg->type; + set_value_embedded_offset (val, value_embedded_offset (arg)); + set_value_pointed_to_offset (val, value_pointed_to_offset (arg)); + return val; + } + return arg; +} + void set_value_component_location (struct value *component, const struct value *whole) Index: gdb/value.h =================================================================== RCS file: /cvs/src/src/gdb/value.h,v retrieving revision 1.161 diff -p -u -r1.161 value.h --- gdb/value.h 7 Jul 2010 16:15:18 -0000 1.161 +++ gdb/value.h 4 Oct 2010 18:11:09 -0000 @@ -710,6 +710,8 @@ extern void preserve_values (struct objf extern struct value *value_copy (struct value *); +extern struct value *value_non_lval (struct value *); + extern void preserve_one_value (struct value *, struct objfile *, htab_t); /* From valops.c */ Index: gdb/testsuite/gdb.base/exprs.exp =================================================================== RCS file: /cvs/src/src/gdb/testsuite/gdb.base/exprs.exp,v retrieving revision 1.19 diff -p -u -r1.19 exprs.exp --- gdb/testsuite/gdb.base/exprs.exp 10 Jun 2010 19:48:19 -0000 1.19 +++ gdb/testsuite/gdb.base/exprs.exp 4 Oct 2010 18:11:09 -0000 @@ -253,3 +253,14 @@ gdb_test "set output-radix 8" ".*" test_expr "print red" "\\$\[0-9\]* = red" test_expr "print/d red" "\\$\[0-9\]* = 0" gdb_test "set output-radix 10" ".*" + +# Pre-/post in-/decrement tests. +gdb_test "set variable v_int = 1" "" +gdb_test "print v_int++" "\\$\[0-9\]* = 1" +gdb_test "print ++v_int" "\\$\[0-9\]* = 3" +gdb_test "print v_int--" "\\$\[0-9\]* = 3" +gdb_test "print --v_int" "\\$\[0-9\]* = 1" +gdb_test "print v_int++ = 5" "Left operand of assignment is not an lvalue." +gdb_test "print ++v_int = 5" "Left operand of assignment is not an lvalue." +gdb_test "print v_int-- = 5" "Left operand of assignment is not an lvalue." +gdb_test "print --v_int = 5" "Left operand of assignment is not an lvalue." --Boundary-00=_q8iqMYRhLSHI+g8--