From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32637 invoked by alias); 3 Nov 2007 09:48:39 -0000 Received: (qmail 32623 invoked by uid 22791); 3 Nov 2007 09:48:37 -0000 X-Spam-Check-By: sourceware.org Received: from zigzag.lvk.cs.msu.su (HELO zigzag.lvk.cs.msu.su) (158.250.17.23) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sat, 03 Nov 2007 09:48:29 +0000 Received: from Debian-exim by zigzag.lvk.cs.msu.su with spam-scanned (Exim 4.50) id 1IoFc4-00081I-GE for gdb-patches@sources.redhat.com; Sat, 03 Nov 2007 12:48:25 +0300 Received: from localhost ([127.0.0.1] helo=ip6-localhost) by zigzag.lvk.cs.msu.su with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA:32) (Exim 4.50) id 1IoFbg-00080x-T8; Sat, 03 Nov 2007 12:47:57 +0300 From: Vladimir Prus To: Nick Roberts Subject: Re: [PATCH] MI: lvalues and variable_editable Date: Sat, 03 Nov 2007 09:48:00 -0000 User-Agent: KMail/1.9.6 Cc: Daniel Jacobowitz , gdb-patches@sources.redhat.com References: <18210.27153.559569.601092@kahikatea.snap.net.nz> <20071102113610.GA16883@caradoc.them.org> <18220.15856.752225.9086@kahikatea.snap.net.nz> In-Reply-To: <18220.15856.752225.9086@kahikatea.snap.net.nz> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline Message-Id: <200711031247.54239.ghost@cs.msu.su> 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: 2007-11/txt/msg00027.txt.bz2 On Saturday 03 November 2007 12:22:56 Nick Roberts wrote: > =A0> > In that case for such a varobj, var->value =3D=3D NULL must indica= te that the > =A0> > memory was inaccessible at the _last_ update, or creation, and it = may no > =A0> > longer be inaccessible. =A0Describing it as "noneditable" may then= be a bit > =A0> > misleading. > =A0>=20 > =A0> The next time it's -var-update'd it may get a new value; presumably > =A0> the IDE does that periodically. >=20 > I guess, in that respect it's no different to -var-evaluate-expression. = =A0Here's > my latest patch for varobj.c. =A0I've removed the test for var->value != =3D NULL in > varobj_set_value as it's already in varobj_editable_p. =A0I've also moved= the > test for var->root->is_valid from varobj_get_attributes to varobj_editabl= e_p. > This patch would require two x two changes to the testsuite: >=20 > FAIL: gdb.mi/mi2-var-child.exp: is weird.int_ptr_ptr.*int_ptr_ptr editable > FAIL: gdb.mi/mi2-var-child.exp: is weird.int_ptr_ptr.*int_ptr_ptr.**int_p= tr_ptr=20 > editable >=20 > editable -> noneditable >=20 > likewise for mi-var-child.exp >=20 > There's also an independent change to mi_cmd_var_assign in mi-cmd-var.c t= hat I > think makes it simpler. >=20 > --=20 > Nick =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 http://www.inet.net.nz/~nickrob >=20 >=20 > 2007-11-03 =A0Nick Roberts =A0 >=20 > =A0=A0=A0=A0=A0=A0=A0=A0* varobj.c (c_variable_editable, cplus_variable_e= ditable) > =A0=A0=A0=A0=A0=A0=A0=A0(java_variable_editable, variable_editable): Dele= te. > =A0=A0=A0=A0=A0=A0=A0=A0(varobj_editable_p): Replace above functions with= one language > =A0=A0=A0=A0=A0=A0=A0=A0independent function. =A0Check for an lvalue. > =A0=A0=A0=A0=A0=A0=A0=A0(varobj_get_attributes, varobj_set_value): Use va= robj_editable_p. > =A0=A0=A0=A0=A0=A0=A0=A0(struct language_specific): Delete variable_edita= ble field. >=20 > =A0=A0=A0=A0=A0=A0=A0=A0* mi-cmd-var.c (mi_cmd_var_assign): Simplify. >=20 >=20 > *** varobj.c=A0=A0=A0=A027 Oct 2007 09:51:19 +1300=A0=A0=A0=A0=A0=A01.96 > --- varobj.c=A0=A0=A0=A003 Nov 2007 21:50:19 +1300=A0=A0=A0=A0=A0=A0 > *************** varobj_set_value (struct varobj *var, ch > *** 887,940 **** > =A0 =A0 struct value *value; > =A0 =A0 int saved_input_radix =3D input_radix; > =A0=20 > ! =A0 if (var->value !=3D NULL && variable_editable (var)) > ! =A0 =A0 { > ! =A0 =A0 =A0 char *s =3D expression; > ! =A0 =A0 =A0 int i; > =A0=20 > ! =A0 =A0 =A0 input_radix =3D 10;=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0/* ALWAYS reset to decimal temporarily */ > ! =A0 =A0 =A0 exp =3D parse_exp_1 (&s, 0, 0); > ! =A0 =A0 =A0 if (!gdb_evaluate_expression (exp, &value)) > ! =A0=A0=A0=A0=A0=A0{ > ! =A0=A0=A0=A0=A0=A0 =A0/* We cannot proceed without a valid expression. = */ > ! =A0=A0=A0=A0=A0=A0 =A0xfree (exp); > ! =A0=A0=A0=A0=A0=A0 =A0return 0; > ! =A0=A0=A0=A0=A0=A0} > !=20 > ! =A0 =A0 =A0 /* All types that are editable must also be changeable. =A0= */ > ! =A0 =A0 =A0 gdb_assert (varobj_value_is_changeable_p (var)); > !=20 > ! =A0 =A0 =A0 /* The value of a changeable variable object must not be la= zy. =A0*/ > ! =A0 =A0 =A0 gdb_assert (!value_lazy (var->value)); > !=20 > ! =A0 =A0 =A0 /* Need to coerce the input. =A0We want to check if the > ! =A0=A0=A0=A0=A0=A0 value of the variable object will be different > ! =A0=A0=A0=A0=A0=A0 after assignment, and the first thing value_assign > ! =A0=A0=A0=A0=A0=A0 does is coerce the input. > ! =A0=A0=A0=A0=A0=A0 For example, if we are assigning an array to a point= er variable we > ! =A0=A0=A0=A0=A0=A0 should compare the pointer with the the array's addr= ess, not with the > ! =A0=A0=A0=A0=A0=A0 array's content. =A0*/ > ! =A0 =A0 =A0 value =3D coerce_array (value); > !=20 > ! =A0 =A0 =A0 /* The new value may be lazy. =A0gdb_value_assign, or=20 > ! =A0=A0=A0=A0=A0=A0 rather value_contents, will take care of this. > ! =A0=A0=A0=A0=A0=A0 If fetching of the new value will fail, gdb_value_as= sign > ! =A0=A0=A0=A0=A0=A0 with catch the exception. =A0*/ > ! =A0 =A0 =A0 if (!gdb_value_assign (var->value, value, &val)) > ! =A0=A0=A0=A0=A0=A0return 0; > ! =A0 =A0 =A0 > ! =A0 =A0 =A0 /* If the value has changed, record it, so that next -var-u= pdate can > ! =A0=A0=A0=A0=A0=A0 report this change. =A0If a variable had a value of = '1', we've set it > ! =A0=A0=A0=A0=A0=A0 to '333' and then set again to '1', when -var-update= will report this > ! =A0=A0=A0=A0=A0=A0 variable as changed -- because the first assignment = has set the > ! =A0=A0=A0=A0=A0=A0 'updated' flag. =A0There's no need to optimize that,= because return value > ! =A0=A0=A0=A0=A0=A0 of -var-update should be considered an approximation= . =A0*/ > ! =A0 =A0 =A0 var->updated =3D install_new_value (var, val, 0 /* Compare = values. */); > ! =A0 =A0 =A0 input_radix =3D saved_input_radix; > ! =A0 =A0 =A0 return 1; > =A0 =A0 =A0 } > =A0=20 > ! =A0 return 0; > =A0 } > =A0=20 > =A0 /* Returns a malloc'ed list with all root variable objects */ > --- 872,920 ---- > =A0 =A0 struct value *value; > =A0 =A0 int saved_input_radix =3D input_radix; I suggest gdb_assert (varobj_is_editable_p (var)); here, so clearly document the assumptions the trailing code makes. > ! =A0 char *s =3D expression; > ! =A0 int i; > =A0=20 > ! =A0 input_radix =3D 10;=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0/* ALWAYS reset= to decimal temporarily */ > ! =A0 exp =3D parse_exp_1 (&s, 0, 0); > ! =A0 if (!gdb_evaluate_expression (exp, &value)) > ! =A0 =A0 { > ! =A0 =A0 =A0 /* We cannot proceed without a valid expression. */ > ! =A0 =A0 =A0 xfree (exp); > ! =A0 =A0 =A0 return 0; > =A0 =A0 =A0 } > =A0=20 > ! =A0 /* All types that are editable must also be changeable. =A0*/ > ! =A0 gdb_assert (varobj_value_is_changeable_p (var)); > !=20 > ! =A0 /* The value of a changeable variable object must not be lazy. =A0*/ > ! =A0 gdb_assert (!value_lazy (var->value)); > !=20 > ! =A0 /* Need to coerce the input. =A0We want to check if the > ! =A0 =A0 =A0value of the variable object will be different > ! =A0 =A0 =A0after assignment, and the first thing value_assign > ! =A0 =A0 =A0does is coerce the input. > ! =A0 =A0 =A0For example, if we are assigning an array to a pointer varia= ble we > ! =A0 =A0 =A0should compare the pointer with the the array's address, not= with the > ! =A0 =A0 =A0array's content. =A0*/ > ! =A0 value =3D coerce_array (value); > !=20 > ! =A0 /* The new value may be lazy. =A0gdb_value_assign, or=20 > ! =A0 =A0 =A0rather value_contents, will take care of this. > ! =A0 =A0 =A0If fetching of the new value will fail, gdb_value_assign > ! =A0 =A0 =A0with catch the exception. =A0*/ > ! =A0 if (!gdb_value_assign (var->value, value, &val)) > ! =A0 =A0 return 0; > ! =A0 =A0 =A0 > ! =A0 /* If the value has changed, record it, so that next -var-update can > ! =A0 =A0 =A0report this change. =A0If a variable had a value of '1', we'= ve set it > ! =A0 =A0 =A0to '333' and then set again to '1', when -var-update will re= port this > ! =A0 =A0 =A0variable as changed -- because the first assignment has set = the > ! =A0 =A0 =A0'updated' flag. =A0There's no need to optimize that, because= return value > ! =A0 =A0 =A0of -var-update should be considered an approximation. =A0*/ > ! =A0 var->updated =3D install_new_value (var, val, 0 /* Compare values. = */); > ! =A0 input_radix =3D saved_input_radix; > ! =A0 return 1; > =A0 } > =A0=20 > =A0 /* Returns a malloc'ed list with all root variable objects */ > + int > + varobj_editable_p (struct varobj *var) > + { > + =A0 struct type *type; > + =A0 struct value *value; > +=20 > + =A0 if (CPLUS_FAKE_CHILD (var)) > + =A0 =A0 return 0; > +=20 > + =A0 if (!(var->root->is_valid && var->value && VALUE_LVAL(var->value))) > + =A0 =A0 return 0; I believe this test captures CPLUS_FAKE_CHILD as well, so you don't need to have separate test for that. > + =A0 type =3D get_value_type (var); > +=20 > + =A0 switch (TYPE_CODE (type)) > + =A0 =A0 { > + =A0 =A0 case TYPE_CODE_STRUCT: > + =A0 =A0 case TYPE_CODE_UNION: > + =A0 =A0 case TYPE_CODE_ARRAY: > + =A0 =A0 case TYPE_CODE_FUNC: > + =A0 =A0 case TYPE_CODE_METHOD: > + =A0 =A0 =A0 return 0; > + =A0 =A0 =A0 break; > +=20 > + =A0 =A0 default: > + =A0 =A0 =A0 return 1; > + =A0 =A0 =A0 break; > + =A0 =A0 } > + } > +=20 > =A0 /* Return non-zero if changes in value of VAR > =A0 =A0 =A0must be detected and reported by -var-update. > =A0 =A0 =A0Return zero is -var-update should never report > *** mi-cmd-var.c=A0=A0=A0=A0=A0=A0=A0=A003 Nov 2007 22:17:04 +1300=A0=A0= =A0=A0=A0=A01.40 > --- mi-cmd-var.c=A0=A0=A0=A0=A0=A0=A0=A003 Nov 2007 12:24:24 +1300=A0=A0= =A0=A0=A0=A0 > *************** mi_cmd_var_assign (char *command, char * > *** 512,527 **** > =A0 =A0 =A0 error (_("mi_cmd_var_assign: Variable object not found")); > =A0=20 > =A0 =A0 /* FIXME: define masks for attributes */ > ! =A0 if (!(varobj_get_attributes (var) & 0x00000001)) > =A0 =A0 =A0 error (_("mi_cmd_var_assign: Variable object is not editable"= )); > =A0=20 > ! =A0 expression =3D xstrdup (argv[1]); > !=20 > ! =A0 if (!varobj_set_value (var, expression)) > ! =A0 =A0 error (_("mi_cmd_var_assign: Could not assign expression to var= iable object")); > =A0=20 > ! =A0 ui_out_field_string (uiout, "value", varobj_get_value (var)); > ! =A0 return MI_CMD_DONE; > =A0 } > =A0=20 > =A0 enum mi_cmd_result > --- 512,529 ---- > =A0 =A0 =A0 error (_("mi_cmd_var_assign: Variable object not found")); > =A0=20 > =A0 =A0 /* FIXME: define masks for attributes */ > ! =A0 if (!varobj_editable_p (var)) > =A0 =A0 =A0 error (_("mi_cmd_var_assign: Variable object is not editable"= )); I presume the FIXME is now stale? > + =A0 else > + =A0 =A0 { > + =A0 =A0 =A0 expression =3D xstrdup (argv[1]); > =A0=20 > ! =A0 =A0 =A0 if (!varobj_set_value (var, expression)) > ! =A0=A0=A0=A0=A0=A0error (_("mi_cmd_var_assign: Could not assign express= ion to variable object")); > =A0=20 > ! =A0 =A0 =A0 ui_out_field_string (uiout, "value", varobj_get_value (var)= ); > ! =A0 =A0 =A0 return MI_CMD_DONE; > ! =A0 =A0 } > =A0 } > =A0=20 > =A0 enum mi_cmd_result Aside from the minor points above, I don't have any objections to this patc= h. Dan, what would you say? - Volodya =20