[RFC/RFA] Fix varobj.c value comparison problems

[RFC/RFA] Fix varobj.c value comparison problems

[Mark Kettenis]

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  <kettenis@gnu.org>

	* 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 <math.h>.
	(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 <math.h>
 
 #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 */

Re: [RFC/RFA] Fix varobj.c value comparison problems

[Daniel Jacobowitz]

On Thu, Feb 17, 2005 at 09:31:50PM -0500, Mark Kettenis wrote:
> 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.

It sounds right to me.

> +  type1 = check_typedef (value_type (val1));
> +  type2 = check_typedef (value_type (val2));
> +  len = TYPE_LENGTH (type1);
> +  if (len != TYPE_LENGTH (type2))
> +    return 0;

Should we just require equal types?  I can't think of a real example,
but hypothetically, if we had a language with tagged unions:
  int kind;
  union { int a; float b; } u __attribute__((tagged(kind)))

and "kind" changed, thereby changing u from "0x80000000" to "whatever
that is as a single-precision float", the client would probably want to
update its display.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFC/RFA] Fix varobj.c value comparison problems

[Andrew Cagney]

Mark, the thing that's missing is an automated testcase.  Having 
something change from -0.0 to +0.0 should do it.

Andrew

Re: [RFC/RFA] Fix varobj.c value comparison problems

[M.M. Kettenis]

Andrew Cagney <cagney@gnu.org> wrote:

> Mark, the thing that's missing is an automated testcase.  Having
> something change from -0.0 to +0.0 should do it.

Ah, but that is the exact the opposite of what triggered this fix:

   NaN != NaN

even though the bit pattern is the same, whereas

   -0.0 == +0.0

even though the bit pattern is different.  I caught the first one with
the existing testsuite although that's merely by accident.  Writing a
test that does this reliable is tricky though as NaNs are handled
differently by different targets.  Testing the second one should be possible.  I'll look into it.

Meanwhile, I've checked in the patch.

Re: [RFC/RFA] Fix varobj.c value comparison problems

[Paul Schlie]

> Mark Kettenis <kettenis@gnu.org> wrote:
>> Andrew Cagney <cagney@gnu.org> wrote:
>> Mark, the thing that's missing is an automated testcase.  Having
>> something change from -0.0 to +0.0 should do it.
>
> Ah, but that is the exact the opposite of what triggered this fix:
>
>   NaN != NaN
>
> even though the bit pattern is the same, whereas
>
>   -0.0 == +0.0

Don't know where the notion of -0.0 == +0.0 is derived from, as they
clearly express different semantics:

 1/-0.0 => -inf
 1/+0.0 => +inf

Therefore clearly aren't equivalent; although +0.0 == abs(-0.0) is.

Nor correspondingly, is it sensible that Nan != NaN; as regardless of
their indeterminate respective values, they express equivalent semantics.

Apparently, a few of the bits of wisdom brought to us by committees.