From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19433 invoked by alias); 17 Nov 2006 17:19:57 -0000 Received: (qmail 19418 invoked by uid 22791); 17 Nov 2006 17:19:53 -0000 X-Spam-Check-By: sourceware.org Received: from main.gmane.org (HELO ciao.gmane.org) (80.91.229.2) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 17 Nov 2006 17:19:39 +0000 Received: from list by ciao.gmane.org with local (Exim 4.43) id 1Gl7MQ-0004hF-84 for gdb-patches@sources.redhat.com; Fri, 17 Nov 2006 18:18:42 +0100 Received: from 73-198.umostel.ru ([82.179.73.198]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Fri, 17 Nov 2006 18:18:42 +0100 Received: from vladimir by 73-198.umostel.ru with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Fri, 17 Nov 2006 18:18:42 +0100 To: gdb-patches@sources.redhat.com From: Vladimir Prus Subject: Re: Variable objects laziness Date: Fri, 17 Nov 2006 17:19:00 -0000 Message-ID: References: <200611141643.25053.vladimir@codesourcery.com> <20275.192.87.1.22.1163760030.squirrel@webmail.xs4all.nl> <200611171344.54155.vladimir@codesourcery.com> <20061117142230.GA29258@nevyn.them.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="nextPart1295820.PO9B2oUvD5" Content-Transfer-Encoding: 7Bit User-Agent: KNode/0.10.2 X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-11/txt/msg00177.txt.bz2 --nextPart1295820.PO9B2oUvD5 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8Bit Content-length: 1724 > Daniel Jacobowitz wrote: >> I think this is the key bit - my_value_equal called value_fetch_lazy, >> the new code calls gdb_value_fetch_lazy. >> >> Vlad, I noticed that the old code used coerce_array and the new one >> doesn't. Is that a problem? > > This should not be a problem on -var-update path, because we never try > to compare values of array types, since for them type_changeable returns > false. > > However, it looks to be a problem on -var-assign path. Given: > > int b[] = {1,2,3}; > int *a = b; > > if we create varobj for 'a' and assign it the value of 'b', we should not > mark this variable as changed. > > I'll double-check this (and other coercions that coerce_array silently > does) and add new test cases as needed. This version of patch fixes the problem and adds new tests. - Volodya Fetch values from memory in a single place, and only fetch the values that are really needed. * varobj.c (struct varobj): Clarify comment. (my_value_equal): Remove. (install_new_value): New function. (type_of_child): Remove. (varobj_create): Use install_new_value. (varobj_set_value): Use value_contents_equal, not my_value_equal. (varobj_update): Use install_new_value. (create_child): Likewise. Inline type_of_child here. (value_of_child): Don't fetch the value. (c_value_of_root): Likewise. (c_value_of_variable): Likewise. (type_changeable): Improve comments. gdb/testsuite/ * gdb.mi/mi-var-cmd.exp: Check -var-update after assignement of arrays and function pointers. * gdb.mi/var-cmd.c: Add declaration necessary for above tests. --nextPart1295820.PO9B2oUvD5 Content-Type: text/x-diff; name="laziness__gdb_mainline.diff" Content-Transfer-Encoding: 8Bit Content-Disposition: attachment; filename="laziness__gdb_mainline.diff" Content-length: 16697 === gdb/testsuite/gdb.mi/mi-var-cmd.exp ================================================================== --- gdb/testsuite/gdb.mi/mi-var-cmd.exp (/patches/gdb/varobj_value_fixes/gdb_mainline) (revision 2144) +++ gdb/testsuite/gdb.mi/mi-var-cmd.exp (/patches/gdb/laziness/gdb_mainline) (revision 2144) @@ -382,6 +382,43 @@ "\\^done,value=\"333\"" \ "assign to lsimple.integer" +mi_gdb_test "-var-update *" \ + "\\^done,changelist=.*" \ + "var update" + +# Check that assignment of function and array values +# promotes the assigned value to function pointer/data +# pointer before comparing with the existing value, +# and does not incorrectly make the value as changed. +mi_gdb_test "-var-assign func do_block_tests" \ + "\\^done,value=\"$hex \"" \ + "assign same value to func" + +mi_gdb_test "-var-update *" \ + "\\^done,changelist=\\\[\\\]" \ + "assign same value to func (update)" + +mi_gdb_test "-var-create array_ptr * array_ptr" \ + "\\^done,name=\"array_ptr\",numchild=\"1\",type=\"int \\*\"" \ + "create global variable array_ptr" + +mi_gdb_test "-var-assign array_ptr array2" \ + "\\^done,value=\"$hex\"" \ + "assign array to pointer" + +mi_gdb_test "-var-update *" \ + "\\^done,changelist=\\\[\{name=\"array_ptr\",in_scope=\"true\",type_changed=\"false\"\}\\\]" \ + "assign array to pointer (update)" + +mi_gdb_test "-var-assign array_ptr array2" \ + "\\^done,value=\"$hex\"" \ + "assign same array to pointer" + +mi_gdb_test "-var-update *" \ + "\\^done,changelist=\\\[\\\]" \ + "assign same array to pointer (update)" + + ###### # End of assign tests ##### === gdb/testsuite/gdb.mi/var-cmd.c ================================================================== --- gdb/testsuite/gdb.mi/var-cmd.c (/patches/gdb/varobj_value_fixes/gdb_mainline) (revision 2144) +++ gdb/testsuite/gdb.mi/var-cmd.c (/patches/gdb/laziness/gdb_mainline) (revision 2144) @@ -106,6 +106,10 @@ b = a; } +int array[] = {1,2,3}; +int array2[] = {4,5,6}; +int *array_ptr = array; + void do_locals_tests () { === gdb/varobj.c ================================================================== --- gdb/varobj.c (/patches/gdb/varobj_value_fixes/gdb_mainline) (revision 2144) +++ gdb/varobj.c (/patches/gdb/laziness/gdb_mainline) (revision 2144) @@ -101,7 +101,9 @@ /* The type of this variable. This may NEVER be NULL. */ struct type *type; - /* The value of this expression or subexpression. This may be NULL. */ + /* The value of this expression or subexpression. This may be NULL. + Invariant: if type_changeable (this) is non-zero, the value is either + NULL, or not lazy. */ struct value *value; /* Did an error occur evaluating the expression or getting its value? */ @@ -202,8 +204,6 @@ static enum varobj_display_formats variable_default_display (struct varobj *); -static int my_value_equal (struct value *, struct value *, int *); - static void vpush (struct vstack **pstack, struct varobj *var); static struct varobj *vpop (struct vstack **pstack); @@ -212,6 +212,9 @@ static char *cppop (struct cpstack **pstack); +static int install_new_value (struct varobj *var, struct value *value, + int initial); + /* Language-specific routines. */ static enum varobj_languages variable_language (struct varobj *var); @@ -226,8 +229,6 @@ static struct value *value_of_child (struct varobj *parent, int index); -static struct type *type_of_child (struct varobj *var); - static int variable_editable (struct varobj *var); static char *my_value_of_variable (struct varobj *var); @@ -445,6 +446,7 @@ { char *p; enum varobj_languages lang; + struct value *value; /* Parse and evaluate the expression, filling in as much of the variable's data as possible */ @@ -505,19 +507,19 @@ /* We definitively need to catch errors here. If evaluate_expression succeeds we got the value we wanted. But if it fails, we still go on with a call to evaluate_type() */ - if (gdb_evaluate_expression (var->root->exp, &var->value)) + if (gdb_evaluate_expression (var->root->exp, &value)) { /* no error */ - if (value_lazy (var->value)) - gdb_value_fetch_lazy (var->value); } else - var->value = evaluate_type (var->root->exp); + value = evaluate_type (var->root->exp); - release_value (var->value); + release_value (value); - var->type = value_type (var->value); + var->type = value_type (value); + install_new_value (var, value, 1 /* Initial assignment */); + /* Set language info */ lang = variable_language (var); var->root->lang = languages[lang]; @@ -826,8 +828,29 @@ return 0; } - if (!my_value_equal (var->value, value, &error)) + /* All types that are editable must also be changeable. */ + gdb_assert (type_changeable (var)); + + /* The value of a changeable variable object must not be lazy. */ + gdb_assert (!value_lazy (var->value)); + + /* Need to coerce the input. We want to check if the + value of the variable object will be different + after assignment, and the first thing value_assign + does is coercing the input. + For example, it means that if assigning array to + a pointer variable, we'll be comparing pointer with array's + address, not pointer with array's content. */ + value = coerce_array (value); + + if (!value_contents_equal (var->value, value)) var->updated = 1; + + /* The new value may be lazy. gdb_value_assign, or + rather value_contents, will take care of this. + It might throw, but unlike var-update for -var-assign + there's just one variable we're working it, so we don't + need to catch the exception here. */ if (!gdb_value_assign (var->value, value, &val)) return 0; value_free (var->value); @@ -871,6 +894,101 @@ return rootcount; } +/* Assign a new value to a variable object. If INITIAL is non-zero, + this is the first assignement after the variable object was just + created, or changed type. In that case, just assign the value + and return 0. + Otherwise, assign the value and if type_changeable returns non-zero, + find if the new value is different from the current value. + Return 1 if so, and 0 is the values are equal. */ +static int +install_new_value (struct varobj *var, struct value *value, int initial) +{ + int changeable; + int need_to_fetch; + int changed = 0; + + var->error = 0; + /* We need to know the varobj's type to decide if the value should + be fetched or not. C++ fake children (public/protected/private) don't have + a type. */ + gdb_assert (var->type || CPLUS_FAKE_CHILD (var)); + changeable = type_changeable (var); + need_to_fetch = changeable; + + if (var->type && TYPE_CODE (var->type) == TYPE_CODE_UNION) + /* For unions, we need to fetch the value implicitly because + of implementation of union member fetch. When gdb + creates a value for a field and the value of the enclosing + structure is not lazy, it immediately copies the necessary + bytes from the enclosing values. If the enclosing value is + lazy, the call to value_fetch_lazy on the field will read + the data from memory. For unions, that means we'll read the + same memory more than once, which is not desirable. So + fetch now. */ + need_to_fetch = 1; + + /* The new value might be lazy. If the type is changeable, + that is we'll be comparing values of this type, fetch the + value now. Otherwise, on the next update the old value + will be lazy, which means we've lost that old value. */ + if (need_to_fetch && value && value_lazy (value)) + { + if (!gdb_value_fetch_lazy (value)) + { + var->error = 1; + /* Set the value to NULL, so that for the next -var-update, + we don't try to compare the new value with this value, + that we couldn't even read. */ + value = NULL; + } + else + var->error = 0; + } + + /* If the type is changeable, compare the old and the new values. + If this is the initial assignment, we don't have any old value + to compare with. */ + if (!initial && changeable) + { + /* If the value of the varobj was changed by -var-set-value, then the + value in the varobj and in the target is the same. However, that value + is different from the value that the varobj had after the previous + -var-update. So need to the varobj as changed. */ + if (var->updated) + changed = 1; + else + { + /* Try to compare the values. That requires that both + values are non-lazy. */ + + /* Quick comparison of NULL values. */ + if (var->value == NULL && value == NULL) + /* Equal. */ + ; + else if (var->value == NULL || value == NULL) + changed = 1; + else + { + gdb_assert (!value_lazy (var->value)); + gdb_assert (!value_lazy (value)); + + if (!value_contents_equal (var->value, value)) + changed = 1; + } + } + } + + /* We must always keep the new value, since children depend on it. */ + if (var->value != NULL) + value_free (var->value); + var->value = value; + var->updated = 0; + + return changed; +} + + /* Update the values for a variable and its children. This is a two-pronged attack. First, re-parse the value for the root's expression to see if it's changed. Then go all the way @@ -940,24 +1058,16 @@ vpush (&result, *varp); changed++; } - /* If values are not equal, note that it's changed. - There a couple of exceptions here, though. - We don't want some types to be reported as "changed". */ - else if (type_changeable (*varp) && - ((*varp)->updated || !my_value_equal ((*varp)->value, new, &error))) + + if (install_new_value ((*varp), new, type_changed)) { - vpush (&result, *varp); - (*varp)->updated = 0; + /* If type_changed is 1, install_new_value will never return + non-zero, so we'll never report the same variable twice. */ + gdb_assert (!type_changed); + vpush (&result, (*varp)); changed++; - /* Its value is going to be updated to NEW. */ - (*varp)->error = error; } - /* We must always keep around the new value for this root - variable expression, or we lose the updated children! */ - value_free ((*varp)->value); - (*varp)->value = new; - /* Initialize a stack */ vpush (&stack, NULL); @@ -983,22 +1093,14 @@ /* Update this variable */ new = value_of_child (v->parent, v->index); - if (type_changeable (v) && - (v->updated || !my_value_equal (v->value, new, &error))) - { + if (install_new_value (v, new, 0 /* type not changed */)) + { /* Note that it's changed */ vpush (&result, v); v->updated = 0; changed++; } - /* Its value is going to be updated to NEW. */ - v->error = error; - /* We must always keep new values, since children depend on it. */ - if (v->value != NULL) - value_free (v->value); - v->value = new; - /* Get next child */ v = vpop (&stack); } @@ -1258,15 +1360,14 @@ { struct varobj *child; char *childs_name; + struct value *value; child = new_variable (); /* name is allocated by name_of_child */ child->name = name; child->index = index; - child->value = value_of_child (parent, index); - if ((!CPLUS_FAKE_CHILD (child) && child->value == NULL) || parent->error) - child->error = 1; + value = value_of_child (parent, index); child->parent = parent; child->root = parent->root; childs_name = xstrprintf ("%s.%s", parent->obj_name, name); @@ -1276,9 +1377,21 @@ /* Save a pointer to this child in the parent */ save_child_in_parent (parent, child); - /* Note the type of this child */ - child->type = type_of_child (child); + /* Compute the type of the child. Must do this before + calling install_new_value. */ + if (value != NULL) + /* If the child had no evaluation errors, var->value + will be non-NULL and contain a valid type. */ + child->type = value_type (value); + else + /* Otherwise, we must compute the type. */ + child->type = (*child->root->lang->type_of_child) (child->parent, + child->index); + install_new_value (child, value, 1); + if ((!CPLUS_FAKE_CHILD (child) && child->value == NULL) || parent->error) + child->error = 1; + return child; } @@ -1452,42 +1565,6 @@ return FORMAT_NATURAL; } -/* This function is similar to GDB's value_contents_equal, except that - this one is "safe"; it never longjmps. It determines if the VAL1's - value is the same as VAL2. If for some reason the value of VAR2 - can't be established, *ERROR2 is set to non-zero. */ - -static int -my_value_equal (struct value *val1, struct value *volatile val2, int *error2) -{ - volatile struct gdb_exception except; - - /* As a special case, if both are null, we say they're equal. */ - if (val1 == NULL && val2 == NULL) - return 1; - else if (val1 == NULL || val2 == NULL) - return 0; - - /* The contents of VAL1 are supposed to be known. */ - gdb_assert (!value_lazy (val1)); - - /* Make sure we also know the contents of VAL2. */ - val2 = coerce_array (val2); - TRY_CATCH (except, RETURN_MASK_ERROR) - { - if (value_lazy (val2)) - value_fetch_lazy (val2); - } - if (except.reason < 0) - { - *error2 = 1; - return 0; - } - gdb_assert (!value_lazy (val2)); - - return value_contents_equal (val1, val2); -} - /* FIXME: The following should be generic for any pointer */ static void vpush (struct vstack **pstack, struct varobj *var) @@ -1679,33 +1756,9 @@ value = (*parent->root->lang->value_of_child) (parent, index); - /* If we're being lazy, fetch the real value of the variable. */ - if (value != NULL && value_lazy (value)) - { - /* If we fail to fetch the value of the child, return - NULL so that callers notice that we're leaving an - error message. */ - if (!gdb_value_fetch_lazy (value)) - value = NULL; - } - return value; } -/* What is the type of VAR? */ -static struct type * -type_of_child (struct varobj *var) -{ - - /* If the child had no evaluation errors, var->value - will be non-NULL and contain a valid type. */ - if (var->value != NULL) - return value_type (var->value); - - /* Otherwise, we must compute the type. */ - return (*var->root->lang->type_of_child) (var->parent, var->index); -} - /* Is this variable editable? Use the variable's type to make this determination. */ static int @@ -1721,9 +1774,15 @@ return (*var->root->lang->value_of_variable) (var); } -/* Is VAR something that can change? Depending on language, - some variable's values never change. For example, - struct and unions never change values. */ +/* Return non-zero if changes in value of VAR + must be detected and reported by -var-update. + Return zero is -var-update should never report + changes of such values. This makes sense for structures + (since the changes in children values will be reported separately), + or for artifical objects (like 'public' pseudo-field in C++). + + Return value of 0 means that gdb need not call value_fetch_lazy + for the value of this variable object. */ static int type_changeable (struct varobj *var) { @@ -1899,19 +1958,7 @@ go on */ if (gdb_evaluate_expression (var->root->exp, &new_val)) { - if (value_lazy (new_val)) - { - /* We need to catch errors because if - value_fetch_lazy fails we still want to continue - (after making val->error = 1) */ - /* FIXME: Shouldn't be using value_contents()? The - comment on value_fetch_lazy() says it is only called - from the macro... */ - if (!gdb_value_fetch_lazy (new_val)) - var->error = 1; - else - var->error = 0; - } + var->error = 0; release_value (new_val); } else @@ -2095,8 +2142,8 @@ struct cleanup *old_chain = make_cleanup_ui_file_delete (stb); char *thevalue; - if (value_lazy (var->value)) - gdb_value_fetch_lazy (var->value); + gdb_assert (type_changeable (var)); + gdb_assert (!value_lazy (var->value)); common_val_print (var->value, stb, format_code[(int) var->format], 1, 0, 0); thevalue = ui_file_xstrdup (stb, &dummy); Property changes on: ___________________________________________________________________ Name: csl:base +/all/patches/gdb/varobj_value_fixes/gdb_mainline Name: svk:merge +e7755896-6108-0410-9592-8049d3e74e28:/mirrors/gdb/trunk:154913 --nextPart1295820.PO9B2oUvD5--