This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] fix pre-/post- in-/decrement
- From: Ken Werner <ken at linux dot vnet dot ibm dot com>
- To: "Ulrich Weigand" <uweigand at de dot ibm dot com>, gdb-patches at sourceware dot org, Joel Brobecker <brobecker at adacore dot com>
- Date: Mon, 4 Oct 2010 21:46:50 +0200
- Subject: Re: [patch] fix pre-/post- in-/decrement
- References: <201010041301.o94D1QHV032611@d12av02.megacenter.de.ibm.com>
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
ChangeLog:
2010-10-04 Ken Werner <ken.werner@de.ibm.com>
* value.h (value_non_lval): Declare.
* value.c (value_non_lval): New function.
* eval.c (evaluate_subexp_standard) <BINOP_ASSIGN, BINOP_ASSIGN_MODIFY,
UNOP_PREINCREMENT, UNOP_PREDECREMENT, UNOP_POSTINCREMENT,
UNOP_POSTDECREMENT>: Call value_non_lval to ensure to return a
non-lvalue.
testsuite/ChangeLog:
2010-10-04 Ken Werner <ken.werner@de.ibm.com>
* 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."