[GDB 6.8] Problem using watchpoints with compound objects

[Antony KING <antony.king@st.com>]

[-- Attachment #1: Type: text/plain, Size: 1898 bytes --]

I am investigating a problem with H/W watchpoints on compound objects in
GDB 6.8 whereby they are not being set in the target nor are they being
detected as being changed (once the first problem is fixed). Take for
example the following simple application:

int a[4];
int main (void)
{
  a[0] = 1;
  return 0;
}

Problem 1 - watchpoint is not set
---------------------------------

If I issue "watch a" to GDB then GDB reports that it has set a H/W
watchpoint but when I continue the watchpoint is not being set. This I
believe is due to a problem in update_watchpoints() in the following test:

	      if (v == b->val
		  || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
		      && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))

The if is unnecessary failing since the test "v == b->val" is not taking
into account that b->val may *not* have been set to v earlier on in the
same function. Looking the CVS head this code has been modified so that
this problem is no longer present. Making a similar change to GDB 6.8
seems to fix the problem (see patch1.txt). Is this solution correct ?

Problem 2 - watchpoint is not detected
--------------------------------------

After applying the patch for problem 1 I then encountered a second
problem where a watchpoint is being erroneously dismissed as being
unchanged. I believe this problem is due to the following test in
watchpoint_check():

      if (!value_equal (b->val, new_val))

This test is only comparing the address of the object and not its
contents. I think the test should be revised to the following:

      if (!value_equal (b->val, new_val)
          || !value_contents_equal (b->val, new_val))

assuming that the value_equal() test is still required (otherwise the
value_equal test could be dropped). Looking at the code in the CVS head
the same issue still seems present. Do you think the above is sufficient
to fix the problem ?

Cheers,

Antony.


[-- Attachment #2: gdb.patch --]
[-- Type: text/plain, Size: 1833 bytes --]

--- breakpoint.c@@/INSIGHT-6.8-ST-2.0	2008-09-11 12:00:00.000000000 +0100
+++ breakpoint.c	2009-03-03 14:20:50.232257000 +0000
@@ -898,7 +898,7 @@ update_watchpoint (struct breakpoint *b,
      is different from out-of-scope watchpoint.  */
   if (within_current_scope && b->exp)
     {
-      struct value *v, *next;
+      struct value *v, *result, *next;
 
       /* Evaluate the expression and make sure it's not lazy, so that
 	 after target stops again, we have a non-lazy previous value
@@ -910,18 +910,18 @@ update_watchpoint (struct breakpoint *b,
 	 In addition, we look at all values which were created
 	 during evaluation, and set watchoints at addresses as needed.
 	 Those values are explicitly deleted here.  */
-      v = evaluate_expression (b->exp);
+      result = evaluate_expression (b->exp);
       /* Avoid setting b->val if it's already set.  The meaning of
 	 b->val is 'the last value' user saw, and we should update
 	 it only if we reported that last value to user.  As it
 	 happens, the code that reports it updates b->val directly.  */
       if (b->val == NULL)
-	b->val = v;
-      value_contents (v);
+	b->val = result;
+      value_contents (result);
       value_release_to_mark (mark);
 
       /* Look at each value on the value chain.  */
-      for (; v; v = next)
+      for (v = result; v; v = next)
 	{
 	  /* If it's a memory location, and GDB actually needed
 	     its contents to evaluate the expression, then we
@@ -934,7 +934,7 @@ update_watchpoint (struct breakpoint *b,
 	      /* We only watch structs and arrays if user asked
 		 for it explicitly, never if they just happen to
 		 appear in the middle of some value chain.  */
-	      if (v == b->val
+	      if (v == result
 		  || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
 		      && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
 		{