[GDB 6.8] Problem using watchpoints with compound objects

[GDB 6.8] Problem using watchpoints with compound objects

[Antony KING]

[-- 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))
 		{

Re: [GDB 6.8] Problem using watchpoints with compound objects

[Daniel Jacobowitz]

On Tue, Mar 03, 2009 at 05:38:56PM +0000, Antony KING wrote:
> 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:

What are you setting a watchpoint on?  value_equal is an
implementation of the equality operator in the CLI.  If you have a
pointer, it will only compare the addresses, but for a struct object
it should compare contents.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [GDB 6.8] Problem using watchpoints with compound objects

[Antony KING]

The following is the example I am using:

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

and the GDB command is "watch a", so GDB should be watching the contents
of the array object and not its address. I have also tried the following
test with the same effect:

struct {int m1, m2, m3, m4;} a;
int main (void)
{
  a.m1 = 1;
  return 0;
}

In both cases value_equal is comparing the address of the object and not
the contents. This is caused, I believe, by the following code at the
start of value_equal:

  arg1 = coerce_array (arg1);
  arg2 = coerce_array (arg2);

which is converting the compound objects into pointers. These are then
used in the latter tests of value_equal.

Cheers,

Antony.

Daniel Jacobowitz wrote:
> On Tue, Mar 03, 2009 at 05:38:56PM +0000, Antony KING wrote:
>> 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:
> 
> What are you setting a watchpoint on?  value_equal is an
> implementation of the equality operator in the CLI.  If you have a
> pointer, it will only compare the addresses, but for a struct object
> it should compare contents.

Re: [GDB 6.8] Problem using watchpoints with compound objects

[Daniel Jacobowitz]

On Tue, Mar 03, 2009 at 09:18:02PM +0000, Antony KING wrote:
> struct {int m1, m2, m3, m4;} a;
> int main (void)
> {
>   a.m1 = 1;
>   return 0;
> }
> 
> In both cases value_equal is comparing the address of the object and not
> the contents. This is caused, I believe, by the following code at the
> start of value_equal:
> 
>   arg1 = coerce_array (arg1);
>   arg2 = coerce_array (arg2);
> 
> which is converting the compound objects into pointers. These are then
> used in the latter tests of value_equal.

Arrays are supposed to be handled specially, but I was talking about
structs since that was in your original example.  coerce_array has no
effect on structs.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [GDB 6.8] Problem using watchpoints with compound objects

[Antony KING]

OK. I now understand why the "struct" example below was not working and
my false assumption that coerce_array() was the culprit in both cases (I
checked the array case but assumed it was the same for the struct :-( ).
The reason why it was not working was because I was initially running
the program with the following GDB script:

file a.out
break main
run
watch a
continue

Using the above script I see the following output from GDB (using the
snapshot version 6.8.50.20090303):

> Breakpoint 1 at 0x8048320: file a.c, line 4.
> 
> Breakpoint 1, main () at a.c:4
> 4         a.m1 = 1;
> Hardware watchpoint 2: a
> 
> Program exited normally.

which shows that the watchpoint is not being reported. However if I
replace "break main" with "tbreak main" in the above script I see the
watchpoint being reported, as follows:

> Temporary breakpoint 1 at 0x8048320: file a.c, line 4.
> 
> Temporary breakpoint 1, main () at a.c:4
> 4         a.m1 = 1;
> Hardware watchpoint 2: a
> Hardware watchpoint 2: a
> 
> Old value = {m1 = 0, m2 = 0, m3 = 0, m4 = 0}
> New value = {m1 = 1, m2 = 0, m3 = 0, m4 = 0}
> main () at a.c:5
> 5         return 0;

So in this instance it seems the problem has nothing to do with "a"
being a struct object (as you correctly point out) but as a result of
continuing from a breakpoint; a different GDB issue :-).

However my original query relating to arrays is still valid. If I
perform the same test with "a" defined as an array (as in my original
email) then the effect of coerce_array() in value_equal() remains; the
watchpoint is reported by the H/W but value_equal() will always report
TRUE (since it compares the address of the array and not its contents)
and hence GDB erroneously ignores the watchpoint as unchanged. Therefore
I still think the change to watchpoint_check() is still valid. If you
concur I will submit a patch for this.

[Note that this was tested on RedHat EL3 and EL4 hosts with the same
results.]

Cheers,

Antony.

Daniel Jacobowitz wrote:
> On Tue, Mar 03, 2009 at 09:18:02PM +0000, Antony KING wrote:
>> struct {int m1, m2, m3, m4;} a;
>> int main (void)
>> {
>>   a.m1 = 1;
>>   return 0;
>> }
>>
>> In both cases value_equal is comparing the address of the object and not
>> the contents. This is caused, I believe, by the following code at the
>> start of value_equal:
>>
>>   arg1 = coerce_array (arg1);
>>   arg2 = coerce_array (arg2);
>>
>> which is converting the compound objects into pointers. These are then
>> used in the latter tests of value_equal.
> 
> Arrays are supposed to be handled specially, but I was talking about
> structs since that was in your original example.  coerce_array has no
> effect on structs.

Re: [GDB 6.8] Problem using watchpoints with compound objects

[Daniel Jacobowitz]

On Wed, Mar 04, 2009 at 07:20:13PM +0000, Antony KING wrote:
> However my original query relating to arrays is still valid. If I
> perform the same test with "a" defined as an array (as in my original
> email) then the effect of coerce_array() in value_equal() remains; the
> watchpoint is reported by the H/W but value_equal() will always report
> TRUE (since it compares the address of the array and not its contents)
> and hence GDB erroneously ignores the watchpoint as unchanged. Therefore
> I still think the change to watchpoint_check() is still valid. If you
> concur I will submit a patch for this.

I think I agree, but honestly I'm too confused to be sure; it does not
help that you're working against GDB 6.8 and this code has changed in
HEAD.  I suggest posting the patch, but including a testcase so that
the change in behavior is realy obvious.

-- 
Daniel Jacobowitz
CodeSourcery