From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21145 invoked by alias); 18 Feb 2005 02:32:09 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 20762 invoked from network); 18 Feb 2005 02:31:52 -0000 Received: from unknown (HELO copland.sibelius.xs4all.nl) (24.75.92.210) by sourceware.org with SMTP; 18 Feb 2005 02:31:52 -0000 Received: from copland.sibelius.xs4all.nl (kettenis@localhost.sibelius.xs4all.nl [127.0.0.1]) by copland.sibelius.xs4all.nl (8.13.3/8.13.3) with ESMTP id j1I2Vo00021089 for ; Fri, 18 Feb 2005 03:31:51 +0100 (CET) Received: (from kettenis@localhost) by copland.sibelius.xs4all.nl (8.13.3/8.13.3/Submit) id j1I2VohT030718; Thu, 17 Feb 2005 21:31:50 -0500 (EST) Date: Fri, 18 Feb 2005 15:28:00 -0000 Message-Id: <200502180231.j1I2VohT030718@copland.sibelius.xs4all.nl> From: Mark Kettenis To: gdb-patches@sources.redhat.com Subject: [RFC/RFA] Fix varobj.c value comparison problems X-SW-Source: 2005-02/txt/msg00192.txt.bz2 Here's a patch for the problems with uninitialized floating-point varobj stuff I reported a few days ago. This patch introduces a new function value_contents_equal. That part is pretty "obvious", although one might argue that it should be put in valarith.c. I didn't put it there because this doesn't implement a C operator. The patch then changes my_value_equal in varobj.c to use that new function. I radically simplified the function. I think these simplifications are justified. The function is used to compare the old value of a variable with the new value of a variable. Therefore the value of VAR1 should already be known. I've put in a gdb_assert to make sure this is indeed the case. So we only have to deal with unlazying VAR2. Thus far, it seems that I'm right. This patch fixes the problems I was seeing and doesn't introduce any new failures. If nobody can shoot any holes in my reasoning, I'll check this in in a few days. Mark Index: ChangeLog from Mark Kettenis * value.h (value_contents_equal): New prototype. * value.c (value_contents_equal): New function. * varobj.c: Include "exceptions.h" and "gdb_assert.h". Don't include . (varobj_set_value): Initialize error to zero. (varobj_update): Rename error2 to error and initialize it to zero. Slightly change the wording of some comments. (my_value_equal): Reimplement using TRY_CATCH and value_contents_equal. Index: value.c =================================================================== RCS file: /cvs/src/src/gdb/value.c,v retrieving revision 1.26 diff -u -p -r1.26 value.c --- value.c 14 Feb 2005 18:10:10 -0000 1.26 +++ value.c 18 Feb 2005 02:20:53 -0000 @@ -358,6 +358,26 @@ value_contents_writeable (struct value * return value->aligner.contents; } +/* Return non-zero if VAL1 and VAL2 have the same contents. Note that + this function is different from value_equal; in C the operator == + can return 0 even if the two values being compared are equal. */ + +int +value_contents_equal (struct value *val1, struct value *val2) +{ + struct type *type1; + struct type *type2; + int len; + + type1 = check_typedef (value_type (val1)); + type2 = check_typedef (value_type (val2)); + len = TYPE_LENGTH (type1); + if (len != TYPE_LENGTH (type2)) + return 0; + + return (memcmp (value_contents (val1), value_contents (val2), len) == 0); +} + int value_optimized_out (struct value *value) { Index: value.h =================================================================== RCS file: /cvs/src/src/gdb/value.h,v retrieving revision 1.82 diff -u -p -r1.82 value.h --- value.h 9 Feb 2005 00:04:29 -0000 1.82 +++ value.h 18 Feb 2005 02:20:53 -0000 @@ -186,6 +186,7 @@ extern bfd_byte *value_contents_all_raw extern const bfd_byte *value_contents_all (struct value *); extern int value_fetch_lazy (struct value *val); +extern int value_contents_equal (struct value *val1, struct value *val2); /* If nonzero, this is the value of a variable which does not actually exist in the program. */ Index: varobj.c =================================================================== RCS file: /cvs/src/src/gdb/varobj.c,v retrieving revision 1.48 diff -u -p -r1.48 varobj.c --- varobj.c 11 Feb 2005 04:06:09 -0000 1.48 +++ varobj.c 18 Feb 2005 02:20:56 -0000 @@ -18,14 +18,16 @@ Boston, MA 02111-1307, USA. */ #include "defs.h" +#include "exceptions.h" #include "value.h" #include "expression.h" #include "frame.h" #include "language.h" #include "wrapper.h" #include "gdbcmd.h" + +#include "gdb_assert.h" #include "gdb_string.h" -#include #include "varobj.h" @@ -784,8 +786,8 @@ int varobj_set_value (struct varobj *var, char *expression) { struct value *val; - int error; int offset = 0; + int error = 0; /* The argument "expression" contains the variable's new value. We need to first construct a legal expression for this -- ugh! */ @@ -875,10 +877,10 @@ int varobj_update (struct varobj **varp, struct varobj ***changelist) { int changed = 0; + int error = 0; int type_changed; int i; int vleft; - int error2; struct varobj *v; struct varobj **cv; struct varobj **templist = NULL; @@ -928,14 +930,13 @@ varobj_update (struct varobj **varp, str 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, &error2))) + ((*varp)->updated || !my_value_equal ((*varp)->value, new, &error))) { vpush (&result, *varp); (*varp)->updated = 0; changed++; - /* error2 replaces var->error since this new value - WILL replace the old one. */ - (*varp)->error = error2; + /* Its value is going to be updated to NEW. */ + (*varp)->error = error; } /* We must always keep around the new value for this root @@ -969,16 +970,15 @@ varobj_update (struct varobj **varp, str /* Update this variable */ new = value_of_child (v->parent, v->index); if (type_changeable (v) && - (v->updated || !my_value_equal (v->value, new, &error2))) + (v->updated || !my_value_equal (v->value, new, &error))) { /* Note that it's changed */ vpush (&result, v); v->updated = 0; changed++; } - /* error2 replaces v->error since this new value - WILL replace the old one. */ - v->error = error2; + /* 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) @@ -1438,60 +1438,40 @@ variable_default_display (struct varobj return FORMAT_NATURAL; } -/* This function is similar to gdb's value_equal, except that this - one is "safe" -- it NEVER longjmps. It determines if the VAR's - value is the same as VAL2. */ +/* 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 *val2, int *error2) { - int r, err1, err2; + volatile struct exception except; - *error2 = 0; - /* Special case: NULL values. If both are null, say - they're equal. */ + /* 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; - /* This is bogus, but unfortunately necessary. We must know - exactly what caused an error -- reading val1 or val2 -- so - that we can really determine if we think that something has changed. */ - err1 = 0; - err2 = 0; - /* We do need to catch errors here because the whole purpose - is to test if value_equal() has errored */ - if (!gdb_value_equal (val1, val1, &r)) - err1 = 1; - - if (!gdb_value_equal (val2, val2, &r)) - *error2 = err2 = 1; + /* The contents of VAL1 are supposed to be known. */ + gdb_assert (!value_lazy (val1)); - if (err1 != err2) - return 0; - - if (!gdb_value_equal (val1, val2, &r)) + /* Make sure we also know the contents of VAL2. */ + val2 = coerce_array (val2); + TRY_CATCH (except, RETURN_MASK_ERROR) { - /* An error occurred, this could have happened if - either val1 or val2 errored. ERR1 and ERR2 tell - us which of these it is. If both errored, then - we assume nothing has changed. If one of them is - valid, though, then something has changed. */ - if (err1 == err2) - { - /* both the old and new values caused errors, so - we say the value did not change */ - /* This is indeterminate, though. Perhaps we should - be safe and say, yes, it changed anyway?? */ - return 1; - } - else - { - return 0; - } + if (value_lazy (val2)) + value_fetch_lazy (val2); } + if (except.reason < 0) + { + *error2 = 1; + return 0; + } + gdb_assert (!value_lazy (val2)); - return r; + return value_contents_equal (val1, val2); } /* FIXME: The following should be generic for any pointer */