[patch] Warn on constant value watchpoints

[patch] Warn on constant value watchpoints

[Jan Kratochvil]

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

Hi,

idea came from a discussion with the Firefox maintainer Martin Stransky who had
disfunctional watchpoint on the address of a variable:

(gdb) watch 0x4343548
Watchpoint 1: 70530376


Regards,
Jan

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

2008-06-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* breakpoint.c (watch_command_1): New variable VAL_RESULT.  Fill in
	VAL_RESULT by the existing FETCH_WATCHPOINT_VALUE call.  Ask user for
	constant list VAL_RESULT.

2008-06-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.texinfo (Set Watchpoints): Document constant value watchpoints.

2008-06-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/watchpoint.exp: New test for constant value watchpoints.

--- gdb/breakpoint.c	6 Jun 2008 20:58:08 -0000	1.324
+++ gdb/breakpoint.c	8 Jun 2008 15:18:38 -0000
@@ -5818,7 +5818,7 @@ watch_command_1 (char *arg, int accessfl
   struct symtab_and_line sal;
   struct expression *exp;
   struct block *exp_valid_block;
-  struct value *val, *mark;
+  struct value *val, *mark, *val_result;
   struct frame_info *frame;
   struct frame_info *prev_frame = NULL;
   char *exp_start = NULL;
@@ -5903,10 +5903,23 @@ watch_command_1 (char *arg, int accessfl
   exp_end = arg;
   exp_valid_block = innermost_block;
   mark = value_mark ();
-  fetch_watchpoint_value (exp, &val, NULL, NULL);
+  fetch_watchpoint_value (exp, &val, &val_result, NULL);
   if (val != NULL)
     release_value (val);
 
+  if (from_tty)
+    {
+      struct value *v;
+
+      /* VAL may be unset for unreachable final values.  */
+      for (v = val_result; v; v = value_next (v))
+	if (VALUE_LVAL (v) == lval_memory || VALUE_LVAL (v) == lval_register)
+	  break;
+      if (v == NULL && !query
+		(_("Do you insist on this watchpoint with a constant value? ")))
+	error (_("Watchpoint with a constant value was cancelled."));
+    }
+
   tok = arg;
   while (*tok == ' ' || *tok == '\t')
     tok++;
--- gdb/doc/gdb.texinfo	6 Jun 2008 20:58:08 -0000	1.503
+++ gdb/doc/gdb.texinfo	8 Jun 2008 15:19:53 -0000
@@ -3375,6 +3426,18 @@ This command prints a list of watchpoint
 it is the same as @code{info break} (@pxref{Set Breaks}).
 @end table
 
+If you watch for a change in a numerically entered address you need to
+dereference it as the address itself is just a constant number which will never
+change:
+
+@smallexample
+(@value{GDBP}) watch 0x600850
+Do you insist on this watchpoint with a constant value? (y or n) n
+Watchpoint with a constant value was cancelled.
+(@value{GDBP}) watch *(int *) 0x600850
+Watchpoint 1: *(int *) 6293584
+@end smallexample
+
 @value{GDBN} sets a @dfn{hardware watchpoint} if possible.  Hardware
 watchpoints execute very quickly, and the debugger reports a change in
 value at the exact instruction where the change occurs.  If @value{GDBN}
--- gdb/testsuite/gdb.base/watchpoint.exp	15 Apr 2008 14:33:54 -0000	1.18
+++ gdb/testsuite/gdb.base/watchpoint.exp	8 Jun 2008 15:19:55 -0000
@@ -679,6 +679,18 @@ set prev_timeout $timeout
 set timeout 600	
 verbose "Timeout now 600 sec.\n"
 
+# Test constant-value watchpoints.
+gdb_test "watch 123" "Watchpoint with a constant value was cancelled."	      \
+    "constant watchpoint"						      \
+    "Do you insist on this watchpoint with a constant value\\? \\(y or n\\) " \
+    "n"
+# For unsupported constant-value watchpoints catching we need to reset the
+# breakpoints counter.
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load $binfile
+
 if [initialize] then {
 
     test_simple_watchpoint

Re: [patch] Warn on constant value watchpoints

[Eli Zaretskii]

> Date: Sun, 8 Jun 2008 17:53:02 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> 
> --tKW2IUtsqtDRztdT
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> 
> Hi,
> 
> idea came from a discussion with the Firefox maintainer Martin Stransky who had
> disfunctional watchpoint on the address of a variable:
> 
> (gdb) watch 0x4343548
> Watchpoint 1: 70530376

Should we allow such watchpoints? under what circumstances are they
useful?

> +      if (v == NULL && !query
> +		(_("Do you insist on this watchpoint with a constant value? ")))

I think a better text for this question would be

  Really watch constant value %s?

> +If you watch for a change in a numerically entered address you need to
> +dereference it as the address itself is just a constant number which will never
> +change:
> +
> +@smallexample
> +(@value{GDBP}) watch 0x600850
> +Do you insist on this watchpoint with a constant value? (y or n) n
> +Watchpoint with a constant value was cancelled.
> +(@value{GDBP}) watch *(int *) 0x600850
> +Watchpoint 1: *(int *) 6293584
> +@end smallexample

Thanks, but please mention in the text that GDB asks for confirmation
because watching a constant value is not generally useful.  Otherwise,
the example might not be fully understood.

Other than that, the patch for the manual is approved (assuming the
code is approved).

Re: [patch] Warn on constant value watchpoints

[Daniel Jacobowitz]

On Sun, Jun 08, 2008 at 09:04:43PM +0300, Eli Zaretskii wrote:
> > (gdb) watch 0x4343548
> > Watchpoint 1: 70530376
> 
> Should we allow such watchpoints? under what circumstances are they
> useful?

In my opinion, we should not allow such watchpoints.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Warn on constant value watchpoints

[Eli Zaretskii]

> Date: Sun, 8 Jun 2008 14:09:09 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Jan Kratochvil <jan.kratochvil@redhat.com>, gdb-patches@sourceware.org
> 
> On Sun, Jun 08, 2008 at 09:04:43PM +0300, Eli Zaretskii wrote:
> > > (gdb) watch 0x4343548
> > > Watchpoint 1: 70530376
> > 
> > Should we allow such watchpoints? under what circumstances are they
> > useful?
> 
> In my opinion, we should not allow such watchpoints.

I agree.  Does anyone disagree?  If not, Jan, could you rework your
patch accordingly?

Re: [patch] Warn on constant value watchpoints

[Jan Kratochvil]

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

On Sun, 08 Jun 2008 20:49:25 +0200, Eli Zaretskii wrote:
> > Date: Sun, 8 Jun 2008 14:09:09 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: Jan Kratochvil <jan.kratochvil@redhat.com>, gdb-patches@sourceware.org
> > 
> > On Sun, Jun 08, 2008 at 09:04:43PM +0300, Eli Zaretskii wrote:
> > > > (gdb) watch 0x4343548
> > > > Watchpoint 1: 70530376
> > > 
> > > Should we allow such watchpoints? under what circumstances are they
> > > useful?
> > 
> > In my opinion, we should not allow such watchpoints.
> 
> I agree.  Does anyone disagree?  If not, Jan, could you rework your
> patch accordingly?

I hope there is no longer any disagreement but requesting an approval.


Thanks,
Jan

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

2008-06-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* breakpoint.c (watch_command_1): New variable VAL_RESULT.  Fill in
	VAL_RESULT by the existing FETCH_WATCHPOINT_VALUE call.  Refuse
	constant VAL_RESULT list watchpoints.

2008-06-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.texinfo (Set Watchpoints): Document constant value watchpoints.

2008-06-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/watchpoint.exp: New test for constant value watchpoints.

--- gdb/breakpoint.c	6 Jun 2008 20:58:08 -0000	1.324
+++ gdb/breakpoint.c	8 Jun 2008 19:11:37 -0000
@@ -5818,7 +5818,7 @@ watch_command_1 (char *arg, int accessfl
   struct symtab_and_line sal;
   struct expression *exp;
   struct block *exp_valid_block;
-  struct value *val, *mark;
+  struct value *val, *mark, *val_result;
   struct frame_info *frame;
   struct frame_info *prev_frame = NULL;
   char *exp_start = NULL;
@@ -5903,10 +5903,28 @@ watch_command_1 (char *arg, int accessfl
   exp_end = arg;
   exp_valid_block = innermost_block;
   mark = value_mark ();
-  fetch_watchpoint_value (exp, &val, NULL, NULL);
+  fetch_watchpoint_value (exp, &val, &val_result, NULL);
   if (val != NULL)
     release_value (val);
 
+  /* VAL may be unset for unreachable final values.  */
+  while (val_result != NULL)
+    {
+      if (VALUE_LVAL (val_result) == lval_memory
+	  || VALUE_LVAL (val_result) == lval_register)
+	break;
+      val_result = value_next (val_result);
+    }
+  if (val_result == NULL)
+    {
+      int len;
+      
+      len = exp_end - exp_start;
+      while (len > 0 && isspace (exp_start[len - 1]))
+	len--;
+      error (_("Cannot watch constant value %.*s."), len, exp_start);
+    }
+
   tok = arg;
   while (*tok == ' ' || *tok == '\t')
     tok++;
--- gdb/doc/gdb.texinfo	6 Jun 2008 20:58:08 -0000	1.503
+++ gdb/doc/gdb.texinfo	8 Jun 2008 19:12:15 -0000
@@ -3375,6 +3375,17 @@ This command prints a list of watchpoint
 it is the same as @code{info break} (@pxref{Set Breaks}).
 @end table
 
+If you watch for a change in a numerically entered address you need to
+dereference it as the address itself is just a constant number which will never
+change.  @value{GDBN} refuses to create a never invokable watchpoint:
+
+@smallexample
+(@value{GDBP}) watch 0x600850
+Cannot watch constant value 0x600850.
+(@value{GDBP}) watch *(int *) 0x600850
+Watchpoint 1: *(int *) 6293584
+@end smallexample
+
 @value{GDBN} sets a @dfn{hardware watchpoint} if possible.  Hardware
 watchpoints execute very quickly, and the debugger reports a change in
 value at the exact instruction where the change occurs.  If @value{GDBN}
--- gdb/testsuite/gdb.base/watchpoint.exp	15 Apr 2008 14:33:54 -0000	1.18
+++ gdb/testsuite/gdb.base/watchpoint.exp	8 Jun 2008 19:12:18 -0000
@@ -679,6 +679,17 @@ set prev_timeout $timeout
 set timeout 600	
 verbose "Timeout now 600 sec.\n"
 
+# Test constant-value watchpoints.
+gdb_test "watch 123" "Cannot watch constant value 123." "constant watchpoint"
+gdb_test "watch 456 if 1 == 2" "Cannot watch constant value 456." \
+	 "constant watchpoint with a condition"
+# For unsupported constant-value watchpoints catching we need to reset the
+# breakpoints counter.
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load $binfile
+
 if [initialize] then {
 
     test_simple_watchpoint

Re: [patch] Warn on constant value watchpoints

[Jan Kratochvil]

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

On Sun, 08 Jun 2008 21:27:55 +0200, Jan Kratochvil wrote:
...
> I hope there is no longer any disagreement but requesting an approval.

Small update, while I see no countercase it should be more safe for future GDB
changes.


Regards,
Jan

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

2008-06-09  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* breakpoint.c (watch_command_1): New variable VAL_RESULT.  Fill in
	VAL_RESULT by the existing FETCH_WATCHPOINT_VALUE call.  Refuse
	constant VAL_RESULT list watchpoints.

2008-06-09  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.texinfo (Set Watchpoints): Document constant value watchpoints.

2008-06-09  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/watchpoint.exp: New test for constant value watchpoints.

--- gdb/breakpoint.c	6 Jun 2008 20:58:08 -0000	1.324
+++ gdb/breakpoint.c	9 Jun 2008 06:00:53 -0000
@@ -5818,7 +5818,7 @@ watch_command_1 (char *arg, int accessfl
   struct symtab_and_line sal;
   struct expression *exp;
   struct block *exp_valid_block;
-  struct value *val, *mark;
+  struct value *val, *mark, *val_result;
   struct frame_info *frame;
   struct frame_info *prev_frame = NULL;
   char *exp_start = NULL;
@@ -5903,7 +5903,27 @@ watch_command_1 (char *arg, int accessfl
   exp_end = arg;
   exp_valid_block = innermost_block;
   mark = value_mark ();
-  fetch_watchpoint_value (exp, &val, NULL, NULL);
+  fetch_watchpoint_value (exp, &val, &val_result, NULL);
+
+  /* VAL may be unset for unreachable final values.  */
+  while (val_result != NULL)
+    {
+      if (VALUE_LVAL (val_result) == lval_memory
+	  || VALUE_LVAL (val_result) == lval_register)
+	break;
+      val_result = value_next (val_result);
+    }
+  if (val_result == NULL)
+    {
+      int len;
+      
+      len = exp_end - exp_start;
+      while (len > 0 && isspace (exp_start[len - 1]))
+	len--;
+      error (_("Cannot watch constant value %.*s."), len, exp_start);
+    }
+
+  /* Break the VAL_RESULT values chain only after its check above.  */
   if (val != NULL)
     release_value (val);
 
--- gdb/doc/gdb.texinfo	6 Jun 2008 20:58:08 -0000	1.503
+++ gdb/doc/gdb.texinfo	9 Jun 2008 06:01:37 -0000
@@ -3375,6 +3375,17 @@ This command prints a list of watchpoint
 it is the same as @code{info break} (@pxref{Set Breaks}).
 @end table
 
+If you watch for a change in a numerically entered address you need to
+dereference it as the address itself is just a constant number which will never
+change.  @value{GDBN} refuses to create a never invokable watchpoint:
+
+@smallexample
+(@value{GDBP}) watch 0x600850
+Cannot watch constant value 0x600850.
+(@value{GDBP}) watch *(int *) 0x600850
+Watchpoint 1: *(int *) 6293584
+@end smallexample
+
 @value{GDBN} sets a @dfn{hardware watchpoint} if possible.  Hardware
 watchpoints execute very quickly, and the debugger reports a change in
 value at the exact instruction where the change occurs.  If @value{GDBN}
--- gdb/testsuite/gdb.base/watchpoint.exp	15 Apr 2008 14:33:54 -0000	1.18
+++ gdb/testsuite/gdb.base/watchpoint.exp	9 Jun 2008 06:01:40 -0000
@@ -679,6 +679,17 @@ set prev_timeout $timeout
 set timeout 600	
 verbose "Timeout now 600 sec.\n"
 
+# Test constant-value watchpoints.
+gdb_test "watch 123" "Cannot watch constant value 123." "constant watchpoint"
+gdb_test "watch 456 if 1 == 2" "Cannot watch constant value 456." \
+	 "constant watchpoint with a condition"
+# For unsupported constant-value watchpoints catching we need to reset the
+# breakpoints counter.
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load $binfile
+
 if [initialize] then {
 
     test_simple_watchpoint

Re: [patch] Warn on constant value watchpoints

[Daniel Jacobowitz]

On Mon, Jun 09, 2008 at 08:14:14AM +0200, Jan Kratochvil wrote:
> @@ -5903,7 +5903,27 @@ watch_command_1 (char *arg, int accessfl
>    exp_end = arg;
>    exp_valid_block = innermost_block;
>    mark = value_mark ();
> -  fetch_watchpoint_value (exp, &val, NULL, NULL);
> +  fetch_watchpoint_value (exp, &val, &val_result, NULL);
> +
> +  /* VAL may be unset for unreachable final values.  */
> +  while (val_result != NULL)
> +    {
> +      if (VALUE_LVAL (val_result) == lval_memory
> +	  || VALUE_LVAL (val_result) == lval_register)
> +	break;
> +      val_result = value_next (val_result);
> +    }

val_result is the same as val, except possibly lazy.  value_next
(val_result) will always be NULL, won't it?  The chain goes the other
way - next from the oldest value (*val_chain argument, or the first
value after the saved mark).

If I'm right it seems like this patch will break "watch a + 1".

Also, I don't know how useful this feature is, but what happens with
"watch foo()"?

> +  if (val_result == NULL)
> +    {
> +      int len;
> +      
> +      len = exp_end - exp_start;
> +      while (len > 0 && isspace (exp_start[len - 1]))
> +	len--;
> +      error (_("Cannot watch constant value %.*s."), len, exp_start);
> +    }

If an intermediate value could not be read, val_result may be NULL.
So this will stop us watching such expressions before they become
readable.

> +If you watch for a change in a numerically entered address you need to
> +dereference it as the address itself is just a constant number which will never
> +change.  @value{GDBN} refuses to create a never invokable watchpoint:

never-changing?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Warn on constant value watchpoints

[Eli Zaretskii]

> Date: Thu, 26 Jun 2008 10:27:31 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb-patches@sourceware.org
> 
> > +If you watch for a change in a numerically entered address you need to
> > +dereference it as the address itself is just a constant number which will never
> > +change.  @value{GDBN} refuses to create a never invokable watchpoint:
> 
> never-changing?

"a watchpoint that watches a never-changing value"

Re: [patch] Warn on constant value watchpoints

[Jan Kratochvil]

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

On Thu, 26 Jun 2008 16:27:31 +0200, Daniel Jacobowitz wrote:
> On Mon, Jun 09, 2008 at 08:14:14AM +0200, Jan Kratochvil wrote:
> > @@ -5903,7 +5903,27 @@ watch_command_1 (char *arg, int accessfl
> >    exp_end = arg;
> >    exp_valid_block = innermost_block;
> >    mark = value_mark ();
> > -  fetch_watchpoint_value (exp, &val, NULL, NULL);
> > +  fetch_watchpoint_value (exp, &val, &val_result, NULL);
> > +
> > +  /* VAL may be unset for unreachable final values.  */
> > +  while (val_result != NULL)
> > +    {
> > +      if (VALUE_LVAL (val_result) == lval_memory
> > +	  || VALUE_LVAL (val_result) == lval_register)
> > +	break;
> > +      val_result = value_next (val_result);
> > +    }
> 
> val_result is the same as val, except possibly lazy.  value_next
> (val_result) will always be NULL, won't it?  The chain goes the other
> way - next from the oldest value (*val_chain argument, or the first
> value after the saved mark).

Thanks, a "typo" (it should have been the 3rd argument - VAL_CHAIN).
It had a regression on `watch **valpp' due to it.
This bug affected your other valid objections.


Thanks,
Jan

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

2008-07-10  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* breakpoint.c (fetch_watchpoint_value): New comment on unreachable
	values.
	(watch_command_1): New variable VAL_CHAIN.  Refuse constant watchpoints.
	* gdbtypes.h (TYPE_CODE_FUNC): New comment regarding pointers to it.

2008-07-10  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.texinfo (Set Watchpoints): Document constant value watchpoints.

2008-07-10  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/watchpoint.exp: Call TEST_CONSTANT_WATCHPOINT.
	(test_constant_watchpoint): New function.
	(test_inaccessible_watchpoint): Cleanup (delete) the watchpoint.
	Test also a double-indirection watchpoint.
	gdb.base/watchpoint.c (global_ptr_ptr): New variable.
	(func4): New testing code for GLOBAL_PTR_PTR.

--- ./gdb/breakpoint.c	8 Jul 2008 11:09:40 -0000	1.330
+++ ./gdb/breakpoint.c	10 Jul 2008 08:19:07 -0000
@@ -824,7 +824,15 @@ is_hardware_watchpoint (struct breakpoin
    If VAL_CHAIN is non-NULL, *VAL_CHAIN will be released from the
    value chain.  The caller must free the values individually.  If
    VAL_CHAIN is NULL, all generated values will be left on the value
-   chain.  */
+   chain.
+   
+   Inferior unreachable values return:
+   Inferior `int *intp = NULL;' with `watch *intp':
+     *VALP is NULL, *RESULTP contains lazy LVAL_MEMORY address 0, *VAL_CHAIN
+     contains the *RESULTP element and also INTP as LVAL_MEMORY.
+   Inferior `int **intpp = NULL;' with `watch **intpp':
+     *VALP is NULL, *RESULTP is NULL, *VAL_CHAIN contains lazy LVAL_MEMORY
+     address 0 and also INTPP as LVAL_MEMORY.  */
 
 static void
 fetch_watchpoint_value (struct expression *exp, struct value **valp,
@@ -5832,7 +5840,7 @@ watch_command_1 (char *arg, int accessfl
   struct symtab_and_line sal;
   struct expression *exp;
   struct block *exp_valid_block;
-  struct value *val, *mark;
+  struct value *val, *mark, *val_chain;
   struct frame_info *frame;
   struct frame_info *prev_frame = NULL;
   char *exp_start = NULL;
@@ -5918,6 +5926,27 @@ watch_command_1 (char *arg, int accessfl
   exp_valid_block = innermost_block;
   mark = value_mark ();
   fetch_watchpoint_value (exp, &val, NULL, NULL);
+
+  /* VALUE_MARK gets us the same value as FETCH_WATCHPOINT_VALUE's VAL_CHAIN
+     parameter.  Just this way we do not have to VALUE_FREE the chained VALUEs
+     ourselves.  */
+  for (val_chain = value_mark ();
+       val_chain != mark;
+       val_chain = value_next (val_chain))
+    if ((VALUE_LVAL (val_chain) == lval_memory
+	 && TYPE_CODE (value_type (val_chain)) != TYPE_CODE_FUNC)
+	|| VALUE_LVAL (val_chain) == lval_register)
+      break;
+  if (val_chain == mark)
+    {
+      int len;
+      
+      len = exp_end - exp_start;
+      while (len > 0 && isspace (exp_start[len - 1]))
+	len--;
+      error (_("Cannot watch constant value %.*s."), len, exp_start);
+    }
+  /* Break the values chain only after its check above.  */
   if (val != NULL)
     release_value (val);
 
--- ./gdb/gdbtypes.h	3 May 2008 22:20:13 -0000	1.87
+++ ./gdb/gdbtypes.h	10 Jul 2008 08:19:08 -0000
@@ -69,7 +69,22 @@ enum type_code
     TYPE_CODE_UNION,		/* C union or Pascal variant part */
     TYPE_CODE_ENUM,		/* Enumeration type */
     TYPE_CODE_FLAGS,		/* Bit flags type */
-    TYPE_CODE_FUNC,		/* Function type */
+
+    /* Function type.  It is not a pointer to a function.  Function reference
+       by its name (such as `printf') has this type.  C automatically converts
+       this function type to a pointer to function for any operation except
+       `sizeof (function_type)' or `&function_type' (unary &).
+       `sizeof (function_type)' is undefined in C.  But GCC provides extension
+       (info '(gcc)Pointer Arith') defining its size as 1 byte.  DWARF does not
+       define its size but GDB defines the size the GCC compatible way - GDB
+       function MAKE_FUNCTION_TYPE.  The address itself is not modifiable.
+       As the function type has size 1 but its real value has `sizeof
+       (CORE_ADDR)' we cannot use NOT_LVAL category because the address would
+       not fit in the VALUE_CONTENTS_RAW container of its VALUE.  We use
+       LVAL_MEMORY (and its VALUE_ADDRESS field) for it but we must be careful
+       it is not lvalue, it is the only non-modifiable LVAL_MEMORY.  */
+    TYPE_CODE_FUNC,
+
     TYPE_CODE_INT,		/* Integer type */
 
     /* Floating type.  This is *NOT* a complex type.  Beware, there are parts
--- ./gdb/doc/gdb.texinfo	7 Jul 2008 12:05:30 -0000	1.506
+++ ./gdb/doc/gdb.texinfo	10 Jul 2008 08:19:37 -0000
@@ -3375,6 +3375,18 @@ This command prints a list of watchpoint
 it is the same as @code{info break} (@pxref{Set Breaks}).
 @end table
 
+If you watch for a change in a numerically entered address you need to
+dereference it as the address itself is just a constant number which will never
+change.  @value{GDBN} refuses to create a watchpoint that watches
+a never-changing value:
+
+@smallexample
+(@value{GDBP}) watch 0x600850
+Cannot watch constant value 0x600850.
+(@value{GDBP}) watch *(int *) 0x600850
+Watchpoint 1: *(int *) 6293584
+@end smallexample
+
 @value{GDBN} sets a @dfn{hardware watchpoint} if possible.  Hardware
 watchpoints execute very quickly, and the debugger reports a change in
 value at the exact instruction where the change occurs.  If @value{GDBN}
--- ./gdb/testsuite/gdb.base/watchpoint.c	3 Mar 2008 13:24:12 -0000	1.3
+++ ./gdb/testsuite/gdb.base/watchpoint.c	10 Jul 2008 08:19:39 -0000
@@ -40,6 +40,7 @@ struct foo struct1, struct2, *ptr1, *ptr
 int doread = 0;
 
 char *global_ptr;
+char **global_ptr_ptr;
 
 void marker1 ()
 {
@@ -118,6 +119,10 @@ func4 ()
   buf[0] = 3;
   global_ptr = buf;
   buf[0] = 7;
+  buf[1] = 5;
+  global_ptr_ptr = &global_ptr;
+  buf[0] = 9;
+  global_ptr++;
 }
 
 int main ()
--- ./gdb/testsuite/gdb.base/watchpoint.exp	15 Apr 2008 14:33:54 -0000	1.18
+++ ./gdb/testsuite/gdb.base/watchpoint.exp	10 Jul 2008 08:19:41 -0000
@@ -644,7 +644,21 @@ proc test_watchpoint_and_breakpoint {} {
 	}
     }
 }
-    
+
+proc test_constant_watchpoint {} {
+    global gdb_prompt
+
+	gdb_test "watch 5" "Cannot watch constant value 5." "number is constant"
+	gdb_test "watch marker1" "Cannot watch constant value marker1." \
+		 "marker1 is constant"
+	gdb_test "watch count + 6" ".*atchpoint \[0-9\]+: count \\+ 6"
+	gdb_test "set \$expr_breakpoint_number = \$bpnum" ""
+	gdb_test "delete \$expr_breakpoint_number" ""
+	gdb_test "watch 7 + count" ".*atchpoint \[0-9\]+: 7 \\+ count"
+	gdb_test "set \$expr_breakpoint_number = \$bpnum" ""
+	gdb_test "delete \$expr_breakpoint_number" ""
+}
+
 proc test_inaccessible_watchpoint {} {
     global gdb_prompt
 
@@ -653,7 +667,8 @@ proc test_inaccessible_watchpoint {} {
 
     if [runto func4] then {
 	gdb_test "watch *global_ptr" ".*atchpoint \[0-9\]+: \\*global_ptr"
-	gdb_test "next" ".*global_ptr = buf.*"
+	gdb_test "set \$global_ptr_breakpoint_number = \$bpnum" ""
+	gdb_test "next" ".*global_ptr = buf.*" "global_ptr next"
 	gdb_test_multiple "next" "next over ptr init" {
 	    -re ".*atchpoint \[0-9\]+: \\*global_ptr\r\n\r\nOld value = .*\r\nNew value = 3 .*\r\n.*$gdb_prompt $" {
 		# We can not test for <unknown> here because NULL may be readable.
@@ -666,6 +681,28 @@ proc test_inaccessible_watchpoint {} {
 		pass "next over buffer set"
 	    }
 	}
+	gdb_test "delete \$global_ptr_breakpoint_number" ""
+	gdb_test "watch **global_ptr_ptr" ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr"
+	gdb_test "set \$global_ptr_ptr_breakpoint_number = \$bpnum" ""
+	gdb_test "next" ".*global_ptr_ptr = &global_ptr.*" "gloabl_ptr_ptr next"
+	gdb_test_multiple "next" "next over global_ptr_ptr init" {
+	    -re ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr\r\n\r\nOld value = .*\r\nNew value = 7 .*\r\n.*$gdb_prompt $" {
+		# We can not test for <unknown> here because NULL may be readable.
+		# This test does rely on *NULL != 7.
+		pass "next over global_ptr_ptr init"
+	    }
+	}
+	gdb_test_multiple "next" "next over global_ptr_ptr buffer set" {
+	    -re ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr\r\n\r\nOld value = 7 .*\r\nNew value = 9 .*\r\n.*$gdb_prompt $" {
+		pass "next over global_ptr_ptr buffer set"
+	    }
+	}
+	gdb_test_multiple "next" "next over global_ptr_ptr pointer advance" {
+	    -re ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr\r\n\r\nOld value = 9 .*\r\nNew value = 5 .*\r\n.*$gdb_prompt $" {
+		pass "next over global_ptr_ptr pointer advance"
+	    }
+	}
+	gdb_test "delete \$global_ptr_ptr_breakpoint_number" ""
     }
 }
     
@@ -833,6 +870,17 @@ if [initialize] then {
     }
 
     test_watchpoint_and_breakpoint
+
+    # See above.
+    if [istarget "mips-idt-*"] then {
+	gdb_exit
+	gdb_start
+	gdb_reinitialize_dir $srcdir/$subdir
+	gdb_load $binfile
+	initialize
+    }
+
+    test_constant_watchpoint
 }
 
 # Restore old timeout

Re: [patch] Warn on constant value watchpoints

[Jan Kratochvil]

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

On Thu, 26 Jun 2008 16:27:31 +0200, Daniel Jacobowitz wrote:
> Also, I don't know how useful this feature is, but what happens with
> "watch foo()"?

Hmm, it crashes.  A loop of:

#7736 call_function_by_hand (function=0xc7bcb0, nargs=0, args=0x7fffffffc528) at infcall.c:727
#7737 evaluate_subexp_standard (expect_type=0x0, exp=0xcb2f90, pos=0x7fffffffc9dc, noside=EVAL_NORMAL) at eval.c:1296
#7738 evaluate_subexp (expect_type=0x0, exp=0xcb2f90, pos=0x7fffffffc9dc, noside=EVAL_NORMAL) at eval.c:75
#7739 evaluate_expression (exp=0xcb2f90) at eval.c:165
#7740 gdb_evaluate_expression (exp=0xcb2f90, value=0x7fffffffca68) at wrapper.c:47
#7741 fetch_watchpoint_value (exp=0xcb2f90, valp=0x7fffffffcb18, resultp=0x7fffffffcb10, val_chain=0x7fffffffcb20) at breakpoint.c:844
#7742 update_watchpoint (b=0xc741c0, reparse=0) at breakpoint.c:937
#7743 insert_breakpoints () at breakpoint.c:1265
#7744 proceed (addr=18446744073709551615, siggnal=TARGET_SIGNAL_DEFAULT, step=0) at infrun.c:1229


This patch is the one I propose to be possibly accepted.


Regards,
Jan

[-- Attachment #2: gdb-watchpoint-loop-noisy.patch --]
[-- Type: text/plain, Size: 3165 bytes --]

2008-07-10  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* infrun.c (decrement_int): New function.
	(proceed): Protect against looping from called INSERT_BREAKPOINTS.

2008-07-10  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/watchpoint.exp: Call TEST_WATCHPOINT_CALLING_INFERIOR.
	(test_watchpoint_calling_inferior): New function.

--- ./gdb/infrun.c	8 Jul 2008 10:31:16 -0000	1.285
+++ ./gdb/infrun.c	9 Jul 2008 08:13:28 -0000
@@ -1146,6 +1146,15 @@ prepare_to_proceed (int step)
    over calls to it and cleared when the inferior is started.  */
 static CORE_ADDR prev_pc;
 
+/* MAKE_CLEANUP stub.  */
+static void
+decrement_int (void *arg)
+{
+  int *int_pointer = arg;
+
+  (*int_pointer)--;
+}
+
 /* Basic routine for continuing the program in various fashions.
 
    ADDR is the address to resume at, or -1 for resume where stopped.
@@ -1226,7 +1235,20 @@ proceed (CORE_ADDR addr, enum target_sig
      or if we are stepping over one but we're using displaced stepping
      to do so.  */
   if (! stepping_over_breakpoint || use_displaced_stepping (gdbarch))
-    insert_breakpoints ();
+    {
+      struct cleanup *old_cleanups;
+      static int nested;
+
+      /* `watch func()' will nest as it will try to PROCEED to execute inferior
+	 FUNC to complete UPDATE_WATCHPOINT.  */
+      if (nested)
+	error (_("Aborting an inferior call used from a watchpoint"));
+
+      old_cleanups = make_cleanup (decrement_int, &nested);
+      nested++;
+      insert_breakpoints ();
+      do_cleanups (old_cleanups);
+    }
 
   if (siggnal != TARGET_SIGNAL_DEFAULT)
     stop_signal = siggnal;
--- ./gdb/testsuite/gdb.base/watchpoint.exp	15 Apr 2008 14:33:54 -0000	1.18
+++ ./gdb/testsuite/gdb.base/watchpoint.exp	9 Jul 2008 08:14:07 -0000
@@ -644,6 +644,33 @@ proc test_watchpoint_and_breakpoint {} {
 	}
     }
 }
+
+# Test a deadlock on an inferior call from a watchpoint.
+
+proc test_watchpoint_calling_inferior {} {
+    if [runto func3] then {
+	# Formerly GDB would deadlock while slowly allocating memory/stack.
+	global timeout
+	set prev_timeout $timeout
+	set timeout 10
+	verbose "Timeout now 10 sec.\n"
+
+	# Only hardware watchpoints cause the problem (in INSERT_BREAKPOINTS).
+	gdb_test "set can-use-hw-watchpoints 1" "" \
+	         "Enable hw-watchpoint for a watchpoint with an inferior call"
+
+	gdb_test "watch func1()" "atchpoint \[0-9\]+: func1 \\(\\)" \
+		 "Put a watchpoint with an inferior call"
+	gdb_breakpoint "func4"
+	# Either abort with `Aborting an inferior call used from a watchpoint'
+	# or it may get somehow resolved but we must get back a prompt.
+	gdb_test "continue" "" "Check the watchpoint with an inferior call"
+
+	# Restore old timeout
+	set timeout $prev_timeout
+	verbose "Timeout now $timeout sec.\n"
+    }
+}
     
 proc test_inaccessible_watchpoint {} {
     global gdb_prompt
@@ -833,6 +861,17 @@ if [initialize] then {
     }
 
     test_watchpoint_and_breakpoint
+
+    # See above.
+    if [istarget "mips-idt-*"] then {
+	gdb_exit
+	gdb_start
+	gdb_reinitialize_dir $srcdir/$subdir
+	gdb_load $binfile
+	initialize
+    }
+
+    test_watchpoint_calling_inferior
 }
 
 # Restore old timeout

Re: [patch] Warn on constant value watchpoints

[Jan Kratochvil]

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

On Thu, 10 Jul 2008 11:02:07 +0200, Jan Kratochvil wrote:
> On Thu, 26 Jun 2008 16:27:31 +0200, Daniel Jacobowitz wrote:
> > Also, I don't know how useful this feature is, but what happens with
> > "watch foo()"?
> 
> Hmm, it crashes.

Here is another patch - more simple but it will just silently watch missing
data, IMO wrong.  Just a try to go the other way.


Regards,
Jan

[-- Attachment #2: gdb-watchpoint-loop-silent.patch --]
[-- Type: text/plain, Size: 1628 bytes --]

--- ./gdb/breakpoint.c	8 Jul 2008 11:09:40 -0000	1.330
+++ ./gdb/breakpoint.c	9 Jul 2008 06:14:26 -0000
@@ -841,7 +841,10 @@ fetch_watchpoint_value (struct expressio
   /* Evaluate the expression.  */
   mark = value_mark ();
   result = NULL;
-  gdb_evaluate_expression (exp, &result);
+  /* We want GDB_EVALUATE_EXPRESSION but we also need EVAL_AVOID_SIDE_EFFECTS,
+     at least for a deadlock calling an inferior function in EXP which needs to
+     UPDATE_WATCHPOINT before its PROCEED.  */
+  gdb_evaluate_type (exp, &result);
   new_mark = value_mark ();
   if (mark == new_mark)
     return;
--- ./gdb/wrapper.c	17 Mar 2008 15:05:42 -0000	1.23
+++ ./gdb/wrapper.c	9 Jul 2008 06:14:33 -0000
@@ -44,7 +44,22 @@ gdb_evaluate_expression (struct expressi
 
   TRY_CATCH (except, RETURN_MASK_ERROR)
     {
-      *value = evaluate_expression(exp);
+      *value = evaluate_expression (exp);
+    }
+
+  if (except.reason < 0)
+    return 0;
+  return 1;
+}
+
+int
+gdb_evaluate_type (struct expression *exp, struct value **value)
+{
+  volatile struct gdb_exception except;
+
+  TRY_CATCH (except, RETURN_MASK_ERROR)
+    {
+      *value = evaluate_type (exp);
     }
 
   if (except.reason < 0)
--- ./gdb/wrapper.h	1 Jan 2008 22:53:13 -0000	1.18
+++ ./gdb/wrapper.h	9 Jul 2008 06:14:33 -0000
@@ -29,6 +29,8 @@ extern int gdb_parse_exp_1 (char **, str
 
 extern int gdb_evaluate_expression (struct expression *, struct value **);
 
+extern int gdb_evaluate_type (struct expression *, struct value **);
+
 extern int gdb_value_fetch_lazy (struct value *);
 
 extern int gdb_value_equal (struct value *, struct value *, int *);